-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from gulp and webpack, to vite #2815
base: devel
Are you sure you want to change the base?
Conversation
pygeek
commented
Sep 8, 2024
•
edited
Loading
edited
- Simplify and speed up the build/dev process by replacing Gulp and Webpack with Vite.
- Additional changes in this PR need to be made to enable HMR — an additional lint rule has been added to support this.
Is there a particular reason for the |
I initially settled on that approach, because it was the fastest to prototype the migration—I was having issues with the pre-existing typescript setup. I've updated the code with my preferred setup ( Context: Vite has a constraint where the |
3460378
to
8af9fd7
Compare
Uffizzi Ephemeral Environment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think a switch to Vite would be exciting. It seems to be the preferred modern build tool for React and other frameworks :)
} catch (error) { | ||
console.warn(error); | ||
console.log("No WASM support detected, score estimator falling back to ASM.js mode"); | ||
script.src = "/OGSScoreEstimator/OGSScoreEstimator-0.7.0.asm.js"; | ||
script.src = "//localhost:8080/OGSScoreEstimator/OGSScoreEstimator-0.7.0.asm.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work in other environments? (i.e. user clients)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values were temporarily hardcoded for prototyping.
@@ -1,19 +1,18 @@ | |||
<!doctype html> | |||
<html lang="{{PAGE_LANGUAGE}}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't Vite also have the ability to use variables in html?
https://vitejs.dev/guide/env-and-mode.html#html-env-replacement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was only done for rapid prototyping.
@@ -9,19 +9,15 @@ | |||
"npm": ">=8.5.0" | |||
}, | |||
"scripts": { | |||
"dev": "supervisor -w Gulpfile.js,webpack.config.js,tsconfig.json supervisor -w Gulpfile.js -x gulp --", | |||
"dev": "npx tsx server.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reload on change is definitely an important feature for devs. (Is this what you're referring to with the HMR updates needed?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vite supports live reload by default. However, with HMR (Hot Module Replacement) the specific module which was updated is reloaded, rather than the entire app. https://webpack.js.org/concepts/hot-module-replacement/
In order to get this working, I have to make updates to a few components. Once this is done, Vite will automatically enable HMR. Related eslint rule: https://github.com/ArnaudBarre/eslint-plugin-react-refresh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty excited for this, I've looked into it in the past but it was a bit daunting, thanks for taking it on.
FTR there's some webpack usage within our Makefile.production
that will need to be replaced with the appropriate vite equivalents, probably most notably is the production builds:
online-go.com/Makefile.production
Lines 125 to 132 in 4b40715
dist/ogs.min.js: dist/ogs.js | |
npm run webpack -- --mode=production --optimization-minimize --devtool=source-map | |
dist/vendor.min.js: dist/vendor.js | |
npm run webpack -- --mode=production --optimization-minimize --devtool=source-map | |
dist/ogs.$(VERSION).css: | |
npm run gulp min_styl |
"spellcheck": "cspell \"src/**/*.{ts,tsx}\"", | ||
"minify-index": "gulp minify-index --silent", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI we do use this targe during deployment, https://github.com/online-go/online-go.com/blob/devel/Makefile.production#L48
8af9fd7
to
20d665b
Compare
20d665b
to
a7ed9ee
Compare
This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this. |
still working on this |