Migrate from Webpack to Vite #879
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
client.js
Outdated
| database: process.env.OED_DB_TEST_DATABASE | ||
| host: process.env.POSTGRES_HOST, | ||
| port: process.env.POSTGRES_PORT, | ||
| user: 'postgres', |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
|
Thanks to @hyperupcall for taking this on and helping OED move forward. I'm sorry for not working on this sooner. The project has been focused on v1.0 release. This change is non-trivial and still seems to have an issue associated with it. Given the v1.0 timeline and the need to verify this change does not cause issues at production sites, I am putting this on hold until after v1.0. It should then receive the attention it deserves and I hope we can move forward with it. If anyone wants to look at the issues noted in the PR then that would be great. |
|
@huss No worries, it would be good to get out v1.0 first. I've been meaning to rebase over main and make the commits a bit more nice and fix the last remaining issues, but I have not got around to doing so yet. |
Vite is more strict in the imports that it accepts. The Moment imports must be updated to only default-import Moment (not namespace imports).
Defining the Redux store in the same file as the app creates problems with Vite. There is some issue with circular imports not working properly with HMR (hot module replacement). Upstream tracks this issue at vitejs/vite#3033; a workaround is to define and export the Redux store from a different file. This commit does exactly that to circumvent the circular import issue.
Vite didn't play nice with these dynamic requires for some reason. To make Vite happy, convert the imports to static imports. Also change them to ESM-imports for consistency.
files This makes the formatting consistent with the `.editorconfig` file.
Properly format files as per the `.editorconfig` file.
- Update various `devDependencies` - Add missing `ini` package used in `src/server/models/obvius/processConfigFile.js`
- Use `NotificationSystem` as type (previous value was no longer a type)
`react-notification-system` has a bug in which is specifies React 16 as a `peerDependency`. Since we use React 17, this causes recent versions of `npm` to error on install. Although this can be partially worked around with `--force` or `--legacy-peer-deps`, it is only a workaround. Until upstream fixes this, we can fix this with the `overrides` field. Since React versions are largely backwards-compatible, we simply set the current version (`$react`) of React (and `react-dom`) as a peer-dependency. Similarly, `@formatjs/intl` is not compatible with `typescript@4`. So, update TypeScript so the `peerDependency` is satisfied. With these fixes, passing `--legacy-peer-deps` is no longer required. Yay!
When using Babel, `core-js` is a common dependency used for automatically polyfillying modern ECMAScript features. However, it is a transitive dependency, so it shouldn't be specified manually as it was. We want to uninstall it, but it appears other parts of the codebase use its `escapeHtml` pollyfill. So, replace these uses with the `escape-html` package, then remove `core-js`.
The previous `TimeInterval.js` was tricky because it was used on the server _and_ client. With Vite, the imported `TimeInterval` variables seemed to always be undefined. It appeared to be a bug with `TimeInterval.js` being a file with CommonJS exports. To remedy this, convert the file so it uses ECMAScript modules (thankfully, the technology is now stable). As a result, imports to this file must be updated as well.
hyperupcall
force-pushed
the
improve-build
branch
from
7fdb271 to
60ebc66
Compare
It appears that with Webpack, this improper import was silently fixed. Rollup ostensibly handles it fine, but when running the final bundle, React error 130[1] shows. So, fix this import so the error goes away. [1]: https://legacy.reactjs.org/docs/error-decoder.html/?args%5B%5D=object&args%5B%5D=&invariant=130
The previous version appeared to work fine, but Lodash documentation prefers default import. And, making this change removes the TypeScript 80003 diagnostic[1]. [1]: https://github.com/microsoft/TypeScript/blob/d210074c8844e21662e40e7db27c45d796be31c4/src/compiler/diagnosticMessages.json#L6713
hyperupcall
force-pushed
the
improve-build
branch
from
60ebc66 to
859f772
Compare
src/scripts/devstart.sh
Outdated
| # host (outside the container) | ||
| extra_args="--host" | ||
| fi | ||
|
|
There was a problem hiding this comment.
This wasn't needed before because the Webpack development server automatically listens on the public network interface (not just loopback). We have to tell Vite to do this ourselves.
src/vite.config.js
Outdated
| build: { | ||
| outDir: './server/public', | ||
| commonjsOptions: { | ||
| // exclude: [/TimeInterval.js$/], |
There was a problem hiding this comment.
This // exclude: [/TimeInterval.js$/], comment will be removed in a later commit.
| @@ -2,8 +2,6 @@ | |||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | |||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|
|||
| import { localeData } from 'moment'; | |||
There was a problem hiding this comment.
This import isn't used as the localData variable declared on line 10 shadows it. Remove it as it could be a source of confusion.
src/server/models/Baseline.js
Outdated
| @@ -3,7 +3,7 @@ | |||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | |||
| */ | |||
| const database = require('./database'); | |||
| const { TimeInterval } = require('../../common/TimeInterval'); | |||
| const { TimeInterval } = import('../../common/TimeInterval.mjs'); | |||
There was a problem hiding this comment.
To elaborate a bit more from the extended commit message: Because TimeInterval.mjs uses ESModules, we can only import things from the file using ESModule-style imports (not CommonJS).
docker-compose.yml
Outdated
| # Don't bring this up without the DB | ||
| - "3000:3000" | ||
| - "9229:9229" # Development port; should be commented out for production | ||
| - "9230:9230" |
There was a problem hiding this comment.
This is one of the issues that I'll mention in my update comment: In Docker, Vite does not listen on port 9229 because it is "already in use" - so as a fallback it listens on the port above that - 9230. As a workaround, expose port 9230 so things work.
There was a problem hiding this comment.
This issue has been fixed in eb0f709
As mentioned in the comments of the code, the default chunking strategy
for Rollup seems poor. This adds a bit of code to split up the bundle.
This should decrease loading times, especially when using HTTP/2 and
HTTP/3.
BEFORE:
```txt
> [email protected] client:build
> vite --config ./src/vite.config.js build
vite v4.2.2 building for production...
✓ 1357 modules transformed.
server/public/index.html 0.89 kB
server/public/assets/index-77128f32.css 160.91 kB │ gzip: 24.60 kB
server/public/assets/index-a8f247e1.js 4,886.14 kB │ gzip: 1,494.16 kB
```
AFTER:
```txt
vite v4.2.2 building for production...
✓ 1357 modules transformed.
server/public/index.html 1.49 kB
server/public/assets/index-99e4836c.css 6.24 kB │ gzip: 1.61 kB
server/public/assets/bootstrap-2823c1df.css 154.67 kB │ gzip: 23.17 kB
server/public/assets/react-select-1d923cf1.js 56.04 kB │ gzip: 19.32 kB
server/public/assets/moment-fbc5633a.js 59.89 kB │ gzip: 19.37 kB
server/public/assets/reactstrap-07f50228.js 72.08 kB │ gzip: 20.20 kB
server/public/assets/lodash-42c17880.js 72.53 kB │ gzip: 26.73 kB
server/public/assets/react-dom-657fb334.js 119.15 kB │ gzip: 38.50 kB
server/public/assets/index-1836615c.js 319.62 kB │ gzip: 67.34 kB
server/public/assets/vendor-144eeccd.js 446.03 kB │ gzip: 141.38 kB
server/public/assets/plotly.js-e0e78342.js 3,711.26 kB │ gzip: 1,150.87 kB
```
hyperupcall
force-pushed
the
improve-build
branch
from
859f772 to
d0375a3
Compare
| // For js, only make the largest libraries their own chunks (>=50kB) | ||
| if (fileName.includes('.js')) { | ||
| if(/^(plotly.js|react-dom|reactstrap|lodash|moment|react-select)$/u.test(moduleName)) { | ||
| return moduleName |
There was a problem hiding this comment.
This improved chunking strategy shows that plotly.js takes up 3.7 GB!!! of space over the wire (as listed in the extended commit description). I checked fd64495, and that commit didn't seem to be the cause. Perhaps the issue lies within react-plotly.js - with how it uses the dependency. 🤔
|
I know @hyperupcall plans more work but I wanted to report what happened when I tied to run the latest version:
but on the second install it was all fine.
It also seems it cannot get data from the DB. |
|
Again, thanks to @hyperupcall for taking on this significant task. I've been holding off the package upgrade and merging non-essential PRs so this one could move forward without conflict. I don't want to be pushy but wanted to ask if you have any idea when there will be further work (or possible finishing of this PR)? FYI that I expect more students to be putting in PRs later in this semester and those will be harder to put off. |
|
@huss You're right, I want to get this done ASAP, before the end of this week, honestly. It looks like there are only a few things that remain, and don't hesitate to ping me if there seems to be no progress. I have time right now to finish investigate the remaining errors and stuff |
That's so odd, because 6476534 was supposed to fix that
It looks like the database fails to initialize. Because the
I'll take a look at port |
|
FWIW I tried switching to the development branch, and I get the same "attempting to create database error": Maybe it is possible that changes to the database like df70d75 or 0e3ace8 affected things? it could be that my database is corrupted, so i will have to investigate that |
Oops, I missed that - @ChrisMart21 by any chance was the server not restarted when you pulled in the latest changes / switched branches? That might have been why HMR failed |
|
@huss I've been thinking, since this PR contains many changes that aren't just removing Webpack / adding Vite, it might be better hold off on this PR for now, and make separate PRs for those things (like issues that indirectly fix things for Vite, formatting inconsistencies). That way, people can submit PRs now without having to worry that things will change under their feet. It might also be possible to add Vite, side-by-side to Webpack to smoothen the transition, although I haven't tried that before. What do you think? |
|
@hyperupcall Do I understand correctly that you are proposing to create a PR for the other (non-Vite) changes and then see about a Vite PR? I think it is okay to separate out the work, esp. if it helps you move it along. I would try to review any PR in timely fashion to so it can clear. Thanks for thinking about this and let me know if I have the wrong idea. |
|
@huss Yes! I don't want this to be blocking other things, especially if it's other students doing their first Open Source contribution. I've also been busy lately, so I think this strategy would reduce uncertainty on both ends. Most of the work is already done, I can just cherry-pick the bigger things and turn those into new PRs. Do you think this PR should be closed now, or after I submit most of the new PRs? |
hyperupcall
force-pushed
the
improve-build
branch
from
9bcdf9b to
20efeb2
Compare
hyperupcall
force-pushed
the
improve-build
branch
from
20efeb2 to
32e497f
Compare
|
Thanks to @hyperupcall for continued work. I tried to install this and got: ed-web-1 | For help, see: https://nodejs.org/en/docs/inspector I tired changing the import to be similar to other moment ones: import moment from 'moment'; but it gave the same error. Any ideas? Also, the npm ci build failed on GitHub. Not sure exactly why but it seems the package files are inconsistent. I wonder if this is a first time issue due to the changes. Something to figure out at some point. |
|
@huss When running NodeJS, the For now though, I'm only using this PR so people can better see the progress on the migration. Before I merged from In any case, I'll ping you when this is ready for review again. I wouldn't want you to be spending time reviewing this when it's not quite ready :) |
|
@hyperupcall Thanks for the note. I know I let this sit for a long time in the past so I think I am wanting to move on it whenever I see an update. I think it is correct that I wait until you let us know that this is ready. Thanks for everything. |
|
Superceded by #1262 |
Description
New Edits
Description
This migrates the build system from Webpack to Vite. The web ecosystem as a whole is moving away from Webpack to newer, faster, and more modern solutions like Vite.
Vite is different than Webpack. In general, Webpack is very lenient with what it accepts. For example, when ECMAScript Modules and CommonJS modules are mixed in a weird/incorrect way, Webpack tries to find some way to correctly resolve the imports, construct a module graph, and output the bundle.
On the other hand, Vite will fail fast if there is something wrong with the imports. Internally, vite uses the ESBuild bundler for the development server, and then the Rollup bundler when bundling for production. Since there are two bundlers,
imports andrequiresmust "work"/"be compatible" with both bundlers.Much of the changes in this PR change the
imports to be more precise and to make both Rollup and ESBuild happy. There are still some issues though, as mentioned further down.The webpack npm scripts are replaced with Vite-specific ones:
npm run vite:devandnpm run vite:buildFixes #871
Some statistics about bundle time and size are shown below for comparison:
Webpack vs Vite Stats
Note that the Vite numbers are actually significantly better since updating to Vite 4.3. Vite 4.3 performs 40-76% better in benchmarks compared to version 4.2!
Docker-Compose
Webpack:
docker-compose upVite
docker-compose upBundling
Webpack:
npm run webpack:buildVite:
npm run vite:buildCurrent Issues
As mentioned in cde54ed, Vite seemed to have a weird issue with running on port(this has been fixed in eb0f709 by modifying the9229(it ran on port9230instead). As a workaround, port9230was also exposed in the docker-compose file. Since it seems running the server through docker-compose was the most documented method of running the server, I didn't think this was a big deal? Maybe this should probably still be fixed.docker-compose.ymlfile)For Further Investigation
plotly.jstakes up about3.5gigabytes of space. In the future, this could be addressed.RouteComponent.tsxcould be lazy-loaded (React.lazy) for improved performance. This isn't related to the Vite migration, but the thought came up.Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)