-
Notifications
You must be signed in to change notification settings - Fork 122
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
frontend: Replace create-react-app with vite and vitest #1981
base: main
Are you sure you want to change the base?
Conversation
d09655a
to
c4b8de6
Compare
@@ -2,18 +2,15 @@ | |||
<html lang="en"> | |||
<head> | |||
<meta charset="utf-8" /> | |||
<link rel="icon" href="%PUBLIC_URL%/favicon.ico" /> | |||
<link rel="icon" href="/favicon.ico" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for PUBLIC_URL here as vite handles that nicely
https://vitejs.dev/guide/assets.html
https://vitejs.dev/guide/build.html#public-base-path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try with a different baseURL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it works
this is ready for review @joaquimrocha @illume |
Are you going to get storyshots working? |
There should be a thankyou/co-author added from the other vite PR. |
It could be a good idea to do preparation PR(s) of changes. Changes that can be done to prepare for this one that don't break what's already there. This way the size of the PR could be reduced. |
@@ -37,7 +37,7 @@ export function runCommand( | |||
throw new Error('runCommand only works in Headlamp app mode.'); | |||
} | |||
|
|||
if (process.env.REACT_APP_ENABLE_RUN_CMD !== 'true') { | |||
if (import.meta.env.REACT_APP_ENABLE_RUN_CMD !== 'true') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can support for process.env be kept somehow?
Mostly interested for a future move of the plugins to vite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, added vite-plugin-env-compatible that makes process.env work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revisited this topic. I removed the process env plugin because app itself is using import.meta.env and plugins don't have access to this since all the env variables are statically replaced during build time and process.env is not available at all.
you can check it by printing process in the dev console in the current version of the app
f1a8899
to
c20bd76
Compare
Improved storyshots test, now it generates separate files for each story, instead of putting it in one big file. Moved the i18next-parser version bump into separate commit. Everything else seems to require vite so keeping it in the other (big) commit. Added env plugin to make process.env work |
99f7832
to
b852dc6
Compare
Signed-off-by: Oleksandr Dubenko <[email protected]> Co-authored-by: Mariusz Winnik <[email protected]>
Some performance comparison: npm run startmeasured time from running the command to page finishing loading in the browser CRA cold start: ~23s Vite hot/cold start: ~6s BuildCRA build: 41s same time, no surprises here. patiently waiting for rolldown hot reloadCRA reload: 2s now this is one of the best quality of life improvement, having quick feedback loop while developing frontend is great |
Build output comparison is here: https://gist.github.com/sniok/95165478c77f1bf86e752c7d6f1f9741 It's mostly similar, I don't anything concerning. Unrelated to vite, there is a lot we can do about what's in each chunk. We can improve headlamp startup performance by using more lazy components and improving code splitting |
This PR continues work started in #1696
Thanks @tazo90!
Replaced cra with vite
Remoted storybook storyshot addon because it is deprecated and doesn't work with vite
Updated all tests to use vitest instead of jest
Two tests don't work because of the bug in vitest, it stuggles with cyclic imports sometimes.
headlamp-plugin doesn't work yet, looking into it
changes:
testing: