Webpack 5 upgrade #7351
Webpack 5 upgrade #7351
Conversation
- Rewrite webpack aliased paths like ~terriajs-variables and ~terriajs-mixins to
plain relative paths so that we can run the auto migrator.
This was done using the following shell command:
```
find lib/ -name '*.scss' -exec bash -c 'sed -i "s|~terriajs-variables|$(realpath -s --relative-to=$(dirname "$1") lib/Sass/common/_variables)|" $1' bash {} \;
```
- Run auto migrator
```
find lib/ -name '*.scss' -exec npx sass-migrator module {} \;
```
Instead of using 'raw-loader!' etc in the import path, we now handle these import in webpack config. See webpack.config.make.js.
Removes use of 'raw-loader!' etc in import paths. Instead handle the imports in webpack config.
Webpack literally works only for 'process.env.NODE_ENV'.
| "bottleneck": "^2.19.5", | ||
| "catalog-converter": "^0.0.9", | ||
| "classnames": "^2.3.1", | ||
| "clipboard": "^2.0.8", | ||
| "commander": "^11.1.0 ", | ||
| "commander": "^12.1.0 ", |
There was a problem hiding this comment.
Commit 0a451d1 downgraded commander to 11.1.0 so it would work with node 16.
I'm not against bumping the engine version though, I would prefer to bump engines to node >= 20 so we could "easily" get rid of the security issues in terriajs-server as well while we're at it.
There was a problem hiding this comment.
Agree that we should bump the engine version, will look into it. In this case though commander is only used for a couple of CLI things and the CI hasn't tripped on it. So it should be fine i guess.
There was a problem hiding this comment.
The previous commander problem that caused the downgrade was most likely fixed by commit 88e587c.
| @@ -165,25 +161,22 @@ | |||
| "react-virtual": "~2.3.2", | |||
| "resolve-url-loader": "^5.0.0", | |||
| "retry": "^0.12.0", | |||
| "sass-loader": "^10", | |||
| "sass-loader": "^16.0.3", | |||
There was a problem hiding this comment.
I'm fairly sure this also requires bumping the engine version.
|
I looked through this and besides the things I commented on, the things that are there looks good to me. Would love to get the (approved) CJS to ESM PR merged soon so this PR doesn't end up in limbo. |
| "css-loader": "^2.1.0", | ||
| "css-modules-typescript-loader": "^2.0.4", | ||
| "css-loader": "^7.1.2", | ||
| "terriajs-typings-for-css-modules-loader": "^2.5.2", |
There was a problem hiding this comment.
Non-alphabetical sorting, so this package will be shuffled down to the other terriajs-packages the next time someone runs yarn add.
|
@na9da just noticed that the late revert of polyfill.js in CJS-to-ESM PR changed the |
| const ForkTsCheckerWebpackPlugin = require("fork-ts-checker-webpack-plugin"); | ||
| const ForkTsCheckerNotifierWebpackPlugin = require("fork-ts-checker-notifier-webpack-plugin"); | ||
| const webpack = require("webpack"); | ||
|
|
||
| function configureWebpack( | ||
| /** |
|
Since this repository was just converted to ESM, should the other terriajs-repositories (that aren't forks of something else) also be converted to ESM? I had a look at terriajs-server and it seems quite easy, but I had to upgrade jasmine and yargs. |
Yep, i'll make those changes in this PR. I didn't want to create two successive breaking changes. |
| "babel-plugin-styled-components", | ||
| "@babel/plugin-syntax-dynamic-import", | ||
| "babel-plugin-lodash" | ||
| "babel-plugin-styled-components" |
There was a problem hiding this comment.
Is there anything else controlling babel plugins in the tree, or should the removed babel plugins also be removed from package.json/yarn.lock?
Edit: (I think removing these plugins from package.json/yarn.lock would remove a bunch of warnings, and possibly also remove something from yarn audit so I would love that.)
| "webpack-cli": "^4.10.0", | ||
| "webpack-dev-server": "^4.15.2", | ||
| "worker-loader": "^2.0.0" | ||
| "webpack": "^5.96.1", |
There was a problem hiding this comment.
I've seen in the upgrade guide that webpack 5 contains types, but it was listed under internal improvements.
If there are type annotations in the webpack 5 package, I'm guessing the types/webpack dev dependency for terriajs should be removed, and if there aren't I'm guessing the types/webpack version should also be bumped to 5.x?
| }) | ||
| ); | ||
|
|
||
| // Fork TypeScript type checking to a separate thread |
There was a problem hiding this comment.
I can see the fork-ts-checker-webpack-plugin 1.0 had support for webpack 5 alpha back in 2019 according to the release notes, but I'm guessing things have changed in webpack 5 since that time. Should the fork-ts-checker-webpack-plugin and fork-ts-checker-notifier-webpack-plugin be bumped to the latest version at the same time as upgrading to webpack 5?
There was a problem hiding this comment.
Running yarn gulp build with empty wwwroot/build runtimes:
Webpack 4: 22.3s
Webpack 5 (this branch): 43.8s
Webpack 5 (this branch) + fork-ts-* version 9.x: 21s
so I suggest bumping the versions even if it isn't strictly required.
| "babel-plugin-styled-components", | ||
| "@babel/plugin-syntax-dynamic-import", | ||
| "babel-plugin-lodash" | ||
| "babel-plugin-styled-components" |
There was a problem hiding this comment.
I'm not sure where this comment fits, but commit dfaf253 added a lib/Core/prerequisites.js which seems to be about compiling to ES5 (which is also what Babel does?).
Should some things be removed from that file as well?
| @@ -6,6 +6,7 @@ | |||
| "engines": { | |||
| "node": ">= 16.0.0" | |||
| }, | |||
| "sideEffects": false, | |||
There was a problem hiding this comment.
I read the webpack manual for optimization, and I get the impression this flag helps with reducing the size of the output. Should other terriajs-packages (terriajs-server, thredds-catalog-crawler, etc.) have a package.json that contains sideEffects: false as well?
There was a problem hiding this comment.
Having read a bit more, I realized a module can be imported for side effects only (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#import_a_module_for_its_side_effects_only), and there seems to be quite a few side effecting imports under lib/:
$ git grep 'import "' lib
lib/Core/polyfill.js:import "core-js/features/promise";
lib/Core/prerequisites.js:import "core-js/features/symbol";
lib/Core/prerequisites.js:import "core-js/features/promise";
lib/Core/prerequisites.js:import "regenerator-runtime/runtime";
lib/Core/prerequisites.js:import "core-js/features/global-this";
lib/Models/GlobeOrMap.ts:import "./Feature/ImageryLayerFeatureInfo"; // overrides Cesium's prototype.configureDescriptionFromProperties
lib/ReactViews/Generic/Editor.jsx:import "!!style-loader!css-loader!./editor.skin.min.css"; // Custom borderless skin
!! TRIMMED tinymce imports in Editor.jsx
lib/ReactViews/Generic/Timer/drawTimer.js:import "d3-transition";
lib/ReactViews/StandardUserInterface/StandardUserInterface.tsx:import "inobounce";
lib/ReactViews/Workbench/WorkbenchList.tsx:import "!!style-loader!css-loader!./sortable.css";
lib/ThirdParty/urijs/index.d.ts:import "urijs/src/URITemplate";
- polyfill/prerequisites: Used by TerriaMap. The point of both polyfill and prerequisites seems to be to "pollute" the global namespace with compatibility functions for browsers that do not support them natively (both "Tip" boxes under https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free), won't that functionality break for TerriaMap when setting
sideEffects: false? - css-imports in Editor and WorkbenchList: I'm guessing second tip box from the link in the previous bullet applies to these?
- GlobeOrMap: the only place that overrides
ImageryLayerFeatureInfo. Can the side-effecting import be eliminated by just putting the 38 lines from the imported file directly into GlobeOrMap? - Editor: lots of side effecting imports of tinymce modules, and then Editor is imported lazily by StoryEditor.jsx. Don't understand the details, but looks fragile in general.
- drawTimer: commit 9035320 added the import in 2019 to "fix the buses layer". I don't know what the "buses layer" is, but the versions of the d3-packages were updated in 2024, so it might be possible to change the import to something more specific now.
- StandardUserInterface: inobounce seems to be a function called from the top level, not sure this can be avoided.
- ThirdParty/urijs: Update types/urijs #7435 removes this.
| ); | ||
| import mapConfigBasicJson from "../../wwwroot/test/Magda/map-config-basic.json"; | ||
| import mapConfigV7Json from "../../wwwroot/test/Magda/map-config-v7.json"; | ||
| import mapConfigInlineInitJson from "../../wwwroot/test/Magda/map-config-inline-init.json"; |
There was a problem hiding this comment.
When I run yarn gulp build on this branch, I get this warning just above the prompt after the build completes:
WARNING in ./node_modules/terriajs-cesium/Source/Core/buildModuleUrl.js 50:37-48
Critical dependency: Accessing import.meta directly is unsupported (only property access or destructuring is supported)
@ ./test/Models/TerriaSpec.ts 41:0-72 113:13-27 118:13-27
| @@ -1 +1 @@ | |||
| module.exports = require("./webpack.config.make")(false, true); | |||
| module.exports = require("./webpack.config.make")(false); | |||
There was a problem hiding this comment.
This config disables devMode, was the wrong parameter removed?
| include: path.resolve(cesiumDir, "Source", "Assets"), | ||
| loader: require.resolve("file-loader"), | ||
| type: "javascript/auto" | ||
| test: /\.(DAC)$/i, |
There was a problem hiding this comment.
I think the parenthesis can be omitted, so /\.DAC$/i should be enough.
| include: path.resolve(cesiumDir, "Source", "ThirdParty"), | ||
| loader: require.resolve("file-loader"), | ||
| type: "javascript/auto" | ||
| test: /\.(css)$/i, |
There was a problem hiding this comment.
I think this parenthesis can be omitted too, so /\.css$/i should be enough.
| // import "!!style-loader!css-loader?sourceMap!tinymce/skins/ui/oxide/skin.min.css"; | ||
| import "!!style-loader!css-loader?sourceMap!./editor.skin.min.css"; // Custom borderless skin | ||
| // import "!!style-loader!css-loader!tinymce/skins/ui/oxide/skin.min.css"; | ||
| import "!!style-loader!css-loader!./editor.skin.min.css"; // Custom borderless skin |
There was a problem hiding this comment.
The style-loader seems to have been removed from the webpack config, do these imports work as intended despite that?
| @@ -6,6 +6,7 @@ | |||
| "engines": { | |||
| "node": ">= 16.0.0" | |||
| }, | |||
| "sideEffects": false, | |||
| "repository": { | |||
| "type": "git", | |||
| "url": "http://github.com/TerriaJS/terriajs" | |||
There was a problem hiding this comment.
There is a resolution of terser-webpack-plugin to version 4 a few lines below (introduced in commit 5b27cb2). You might be able to drop the resolution, or at least update the resolution to version 5.
| @import "~terriajs-variables"; | ||
| @import "../../../../Sass/common/mixins"; | ||
| @use "../../../../Sass/common/_variables"; | ||
| @use "../../../../Sass/common/mixins"; |
There was a problem hiding this comment.
Some of these files import mixins, and others import _mixins (see guidance-dot.scss vs this file, both in subdirectories of ReactViews), is that intentional?
This is great, navigating around old unused things is confusing for newcomers.
I don't understand this change in general; the parameters are packed into a record at the caller and then immediately unpacked in the arguments of the callee, so the change is effectively adding braces around what was there previously. I also get a complaint that the JSDoc is invalid because it references
I'm all for improved security so I love this, but just updating to Webpack 5 fixes a bunch of vulnerabilities, and this PR blocks a lot of other things. Can the plugin be used with Webpack 5 so this PR can get merged first, and you then do this rewrite in a separate PR? Edit: I just realized terriajs keeps a fork of Cesium. Isn't it easiest to change the message in that fork, and not rewrite anything at build time?
I had no idea how things were built before looking at this PR and I'm not thrilled about how the generated
If the plugin works with Webpack 5, I think that should be handled in a separate PR because this PR in its current form brings a lot of improvements already. |
| }); | ||
|
|
||
| // Remove Cesium debug mode checks in production. This might slightly improve performance. | ||
| // TODO: It will be good to have it enabled in devMode though, to discover issues with code | ||
| // however doing so currently breaks a few specs. |
What this PR does
Fixes #6998
Depends on TerriaJS/TerriaMap#718 and #7311
Upgrades webpack and other build dependencies to the latest version.
Note: bulk of the changes are from running the saas-migrator that upgrades
.scssfiles to modern api.Changes summary
webpack, ...
plugin-proposal-class-properties,proposal-object-rest-spread,plugin-syntax-dynamic-import. These must now be well supported by browsers.raw-loaderfile-loaderurl-loaderetc with builtin asset modulesraw-loader!file-loaderetc to make it more ESM compliant. Added extra config in webpack to correctly handle imports of static files (like csv and xml).hotreload config. AFAICT this was not being used and required maintaining two different code paths in webpack configuration. If required we can add it back later after testing that it works.configureWebpackparameters with a single options parameter.string-replace-webpack-pluginas it was outdated and has a few vulnerabilities.sass-migratorscript)~terriajs-variablesand~terriajs-mixinswith respective relative path. This was necessary to run the migrator. Also it means simpler webpack config.css-modules-typescript-loaderthat generates TS types for SASS modules is outdated and no longer works with webpack 5. I have made a repo from a fork of another similar package that can generate types. The other alternative would be to use this TS plugin - but not sure if it'll slow down compilation/editing.svg-sprite-loader. It appears to be not maintained and has a bunch of audit warnings.I'll annotate the changes to make review easier.
Test me
Checklist
doc/.