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.
| @@ -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...)
There was a problem hiding this comment.
Ah, I broke it when upgrading other deps but I've now restored it. I only added it so the setup across this, react-app and in theory the editor all align to make it easier for ourselves when coding across different projects.
| }, | ||
| }, | ||
|
|
||
| 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
There was a problem hiding this comment.
I've only copied this over from the previous config; it seems perfectly fine to leave it here as Vite does accept a mode like this. I suppose we could pass it in via --mode {mode} in package.json if you wanted? Seems not to matter.
TS7 does not yet have an API release, so `typescript-eslint` (a fairly major part of eslint) won't work with it. TS6 is still new and we can upgrade again at a later date.
|
I've made the changes! Happy that |
2 participants
No description provided.