Upgrade deps; migrate to Vite - #8
Conversation
There was a problem hiding this comment.
I've locally checked the demo site (https://code-editor.ada-cs.org/), which works perfectly (used localhost for the new version). However, I don't think we'll be able to test the integration without actually deploying this.
Thanks for the migration to vite! I don't know too much about its advantages compared to webpack, but I think just the consistency with react-app is worth the migration.
Interestingly, there's a disconnect between the overall bundle size and the amount of data downloaded on the demo site. Overall bundle size decreased from ~7 MB to ~4.5MB. However, the size of content actually downloaded by the demo site has increased from 886 KB to almost 2MB. I think webpack broke up the dependencies into smaller chunks? Still, I think 2 MB is fine. A nice improvement is that we no longer host source maps, which we did, eg. https://code-editor.ada-cs.org/435.map.js, although this isn't where the reduction in bundle size is coming from, as I've excluded the source maps from the ~7MB figure.
Although not explicit in the commit, when we rebuild the service for the deploy, the nginx version will update from 1.24.0 to 1.30.4 (or a newer version if that's available). It might also be worth fixing this version in the Dockerfile (something to do later, not a change I'm requesting now).
However, please go through the findings and decide if you'd like to address any of my comments. I think the typescript errors would be worth fixing, and restoring the old docker-compose build behaviour so it breaks on typescript errors would also be worth it.
| @@ -4,11 +4,12 @@ WORKDIR /editor | |||
|
|
|||
| COPY package.json /editor/package.json | |||
There was a problem hiding this comment.
Not introduced by this PR, but still quite major so I'll raise it. We never copy the lockfile, so this just installs whatever is allowed by package.json, where we use pretty broad ranges (eg. ^18.3.10, but this will fetch latest available 18.x version). The current state of main, for example, fails to build (but builds successfully once I pass the lockfile).
Using yarn --frozen-lockfile as the command would have yarn fail when the lockfile is missing instead of silently generating a new one.
| @@ -0,0 +1,59 @@ | |||
| import globals from "globals"; | |||
There was a problem hiding this comment.
What is the eslint config used for? Have you been able to execute it successfully? Is the config just here for VSCode? (doesn't even work there for me...)
| "watch": "webpack --watch", | ||
| "serve": "webpack serve" | ||
| "start": "vite --port 3000", | ||
| "build": "vite build", |
There was a problem hiding this comment.
The yarn build command no longer checks for typescript compilation errors (neither does yarn start). I was able to add console.log(a) to index.ts and received no error message (but do receive an error message with the same change on main).
In fact, I get 5 typescipt errors executing npx tsc --noEmit:
Errors Files
1 src/app/Editor.tsx:49
1 src/app/languages/javascript.ts:23
2 src/app/languages/sql.ts:7
1 src/index.tsx:1
I think we should fix these, and make sure docker-compose build fails when there are typescript errors. This is what currently happens on main.
| }, | ||
| }, | ||
|
|
||
| mode: process.env.NODE_ENV === 'production' ? 'production' : 'development', |
There was a problem hiding this comment.
shouldn't we set "production" either in package.json or docker-compose? I'm not sure what difference this would make though, as we already don't emit source maps
No description provided.