Skip to content

[devex] add vite #5260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 20, 2023
Merged

[devex] add vite #5260

merged 2 commits into from
Dec 20, 2023

Conversation

guillaumejparis
Copy link
Member

Add vite for local development

@guillaumejparis guillaumejparis added the filigran team use to identify PR from the Filigran team label Dec 19, 2023
@guillaumejparis guillaumejparis self-assigned this Dec 19, 2023
Comment on lines +10 to +14
settings: {
react: {
version: 'detect',
},
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for the eslint plugin react warning "en passant"

Comment on lines +1 to +18
<!doctype html>
<html lang="en">
<head>
<script>window.BASE_PATH = "%BASE_PATH%"</script>
<title>%APP_TITLE%</title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1">
<meta name="dеѕсrірtіоn" content="%APP_DESCRIPTION%">
<link id="favicon" rel="shortcut icon" href="%APP_FAVICON%">
<link id="manifest" rel="manifest" href="%APP_MANIFEST%">
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
<script type="module" src="/src/front.tsx"></script>
</body>
</html>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Vite to work we MUST have the index.html at the root

Comment on lines 145 to +143
"start": "yarn relay && node builder/dev/dev.js",
"dev": "yarn relay && vite",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first vite is optin with the yarn dev command

Comment on lines +198 to +208
name: 'treat-js-files-as-jsx',
async transform(code, id) {
if (!id.match(/src\/.*\.js$/)) return null;
// Use the exposed transform from vite, instead of directly
// transforming with esbuild
return transformWithEsbuild(code, id, {
loader: 'tsx',
jsx: 'automatic',
});
},
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed because we have js files with jsx inside

@guillaumejparis
Copy link
Member Author

With the addition of Vite, a little performance problem has been spotted : at the first Vite launch a lot of files are downloaded by the browser (every src/ files + dependencies chunks), to ease this some main routes are now lazy loaded, in the future we maybe will use it to improve production first render.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (46c34f9) 65.60% compared to head (840119f) 65.60%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5260   +/-   ##
=======================================
  Coverage   65.60%   65.60%           
=======================================
  Files         472      472           
  Lines       59160    59160           
  Branches     4328     4328           
=======================================
  Hits        38811    38811           
  Misses      20349    20349           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

import * as path from 'node:path';
import relay from 'vite-plugin-relay';

// to avoid multiple reload when discovering new dependencies after a going on a lazy (not precedently) loaded route we pre optmize these dependencies
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid multiple reload AND to avoid changing every mui imports across the app

@guillaumejparis guillaumejparis marked this pull request as ready for review December 19, 2023 15:41
@lndrtrbn lndrtrbn self-requested a review December 20, 2023 08:07
Copy link
Member

@lndrtrbn lndrtrbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally with both start and dev command. Looks good to me for a merge

@labo-flg
Copy link
Member

Tested ok, good job LGTM 🙌

We'll use the remaining time in this dev cycle to stabilize or, if this happens to be really a bad idea, revert the changes.

@guillaumejparis guillaumejparis merged commit 5b26ba0 into master Dec 20, 2023
@guillaumejparis guillaumejparis deleted the improvement/add-vite branch December 20, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants