Conversation
|
Significant change in how the client is used after the migration; no longer dependent on webpack bundling, so there’s a change to performance in how it’s used; should performance optimizations be part of this migration before the task is considered completed, or should it be a second step after the Vite migration? Document potential performance impact, do the migration, then address performance as second issue |
pawiecz
requested review from
cassidyjames,
dom-notion,
juanmrad,
julietshen and
vinaysrao1
as code owners
juanmrad
left a comment
juanmrad
left a comment
There was a problem hiding this comment.
Tested locally -- both server and client start and work correctly. Vite dev server starts in ~235ms and the proxy to the backend works as expected. Production build also completes successfully in ~7s. Nice migration!
A few cleanup items below, but the core migration is solid.
Also to note. The build produces a 3MB Dashboard chunk. Agree with @cassidyjames that performance optimization should be a follow-up issue -- Vite's build.rollupOptions.output.manualChunks could help split this up.
| "react-scripts": { | ||
| "typescript": "^5" | ||
| }, | ||
| "react-dev-utils": { | ||
| "fork-ts-checker-webpack-plugin": "^6.5.3" | ||
| }, |
There was a problem hiding this comment.
The jest config block and the overrides for react-scripts/react-dev-utils are still here. Since we've moved to vitest (configured in vite.config.ts), these are now dead config. Can we clean them up?
There was a problem hiding this comment.
I dropped Jest config block but left the react-scripts-related overrides - detailed explanation below in the Storybook thread.
| "@storybook/addon-interactions": "^8.2.6", | ||
| "@storybook/addon-links": "^8.2.6", | ||
| "@storybook/blocks": "^8.2.6", | ||
| "@storybook/preset-create-react-app": "^8.2.6", |
There was a problem hiding this comment.
@storybook/preset-create-react-app is still listed.
does Storybook still work with Vite, or does this need to be swapped for @storybook/builder-vite?
There was a problem hiding this comment.
Storybook is currently non-operational (scripts build-storybook and storybook fail on current main branch). Issue that causes build failure (handlebars unavailability) shouldn't affect upgraded Storybook.
Swapping just the builder causes conflicts: recent @storybook/builder-vite conflicts with the rest of the current setup and installing compatible version (^8.2.6) works only with Vite <=v6.
If that would be OK with you I'll fix and upgrade Storybook setup in a separate PR (using Vite-specific builder). Also, @storybook/preset-create-react-app pulls react-dev-utils and react-scripts. I would drop them together with their overrides during that upgrade.
Version selection was delegated to ".nvmrc" file where applicable. Fixes: #111
Variable "NODE_ENV" is no longer used in client. It's been replaced with
Vite-specific "MODE" which can be adjusted with "--mode" command line argument
and defaults to:
- "development" for plain "vite" call
- "production" for "build" and "preview" calls
Even though server still uses "GOOGLE_PLACES_API_KEY" variable, client now uses
"VITE_GOOGLE_PLACES_API_KEY" ("VITE_"-prefixed). This way Vite can access this
information.
pawiecz
force-pushed
the
migrate-to-vite
branch
from
6f92989 to
b12c6ea
Compare
pawiecz
force-pushed
the
migrate-to-vite
branch
from
b12c6ea to
e98dbf3
Compare
|
I'm unable to reproduce |
3 participants
Context & Requests for Reviewers
Fixes: #40
Tests
Standard instance setup steps remain unchanged and produce the same results (changes are hidden under
npmscripts abstractions).(Optional) Rollout Plan
Updated dev environments may seem slow on the first start (hence a note in the README
Getting startedsection).