Webpack5 comments#7450
Conversation
…er without a style configured
The eslint-recommended configuration is a subset of recommended, so switch to the recommended configuration and remove the rules that are already part of the recommended configuration. There are a few rules that result in numerous errors, so disable them for now.
Remove the dark-matter image now that it is no longer referenced in the tests. This should have been a part of TerriaJS#7314.
Traverse the file system once, and keep that result in a variable.
Replace the in-file var declarations with let/const. Leave the imports as-is to avoid conflicts with the CJS to ESM upgrade.
I got this warning when running some command: npm warn deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful. and inflight is a dependency of glob, which glob-all depends on. So replace glob-all with fast-glob that both seems fast and is already used by some other dependencies.
This was previously used for loading polyfills from polyfill.io then we switched to core-js.
ESLint: use recommended configuration
This saves the compiler the effort of having to infer the types during compilation, which shortens compilation times. This commit is just the code fixes, but there is also a typescript-eslint rule for this: https://typescript-eslint.io/rules/explicit-module-boundary-types/ The annotations were generated from the fixes in the TypeScript compiler with: ts-fix -t ./tsconfig-node.json -e 9008 --write --ignoreGitStatus --file lib/**/*.ts
These functions do not await anything, so remove the async from them. This will become an error when switching to the typed lint rules in typescript-eslint.
To avoid breaking change to terriamap. We'll make this change when upgrading to webpack5.
Move release guide to separate file
| composes: btn from "../../../../Sass/common/_buttons.scss"; | ||
| // TODO: fix? | ||
| // composes: btn-small from "../../../../Sass/common/_buttons.scss"; | ||
| // composes: btn--left from "../../../../Sass/common/_buttons.scss"; |
There was a problem hiding this comment.
btn--left was an icon definition so it is safe to remove
btn-small wouldn't make any point here as we have all its styles defined/overriden here or in styled-component
| } | ||
|
|
||
| // TODO: fix? | ||
| // .btn-small { |
There was a problem hiding this comment.
This is not used anywhere anymore
Upgrade protomaps leaflet and remove mvt imagery provider
Upgrade upload-artifact action to v4
| "engines": { | ||
| "node": ">= 16.0.0" | ||
| }, | ||
| "sideEffects": false, |
There was a problem hiding this comment.
This is apparently important for tree shaking. There is a granular version of sideEffects where you provide paths, and I think that would make sense since there are so few imports with side effects in the tree currently. Long-term, I'm guessing you want to get rid of side effecting imports.
There was a problem hiding this comment.
It is not always easy to properly setup the list of files, and I don't want to block the webpack upgrade with that
There was a problem hiding this comment.
True, it's already been sitting for 2 months, would be nice to get these big PRs into main.
There was a problem hiding this comment.
Yeah, we should remove this setting. I was mostly testing to see if it broke anything - it doesn't seem to. I think we can revisit this later.
|
Your change to that other package looks straightforward and should be a matter of just merging, and then getting this moving. Do you know what is blocking the progress with getting this merged? The diff view is no longer useful for reviewing after merging main, but besides me wishing for not completely eliminating the sideEffects directive I think what was here before merging main into the PR looked good. |
zoran995
force-pushed
the
webpack5-comments
branch
from
f650e06 to
c7aa95c
Compare
|
@na9da I had to restore |
| new URL("./downloadHrefWorker.js", import.meta.url) | ||
| ); | ||
| const worker = await import( | ||
| //@ts-expect-error: aaa |
There was a problem hiding this comment.
I assume you know the reason you had to put this ts-expect-error in, put that instead of "aaa".
And style wise, no other ts-expect-error in terriajs has a colon after it, and most of them have a space between the // and @ts-expect-error.
I don't know what part of the code this concerns, but it seems worth to put a comment in the code at that place so it doesn't get missed/lost the next time that thing is upgraded. |
zoran995
force-pushed
the
webpack5-comments
branch
from
ca39041 to
ffd19ab
Compare
|
I have cherry-picked and merged all changes from here into #7351, so it is easier and faster to review |
8 participants
What this PR does
Test me
Locally:
Checklist
doc/.