Skip to content

Upgrade deps; migrate to Vite - #8

Open
jacbn wants to merge 2 commits into
mainfrom
improvement/26-07-deps-update
Open

Upgrade deps; migrate to Vite#8
jacbn wants to merge 2 commits into
mainfrom
improvement/26-07-deps-update

Conversation

@jacbn

@jacbn jacbn commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@barna-isaac barna-isaac left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Dockerfile
@@ -4,11 +4,12 @@ WORKDIR /editor

COPY package.json /editor/package.json

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread eslint.config.mjs
@@ -0,0 +1,59 @@
import globals from "globals";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Comment thread package.json
"watch": "webpack --watch",
"serve": "webpack serve"
"start": "vite --port 3000",
"build": "vite build",

@barna-isaac barna-isaac Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread vite.config.ts
},
},

mode: process.env.NODE_ENV === 'production' ? 'production' : 'development',

@barna-isaac barna-isaac Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants