fix(build): make Node.js scripts work with Node.js 20 #4558
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maxpatiiuk
commented
Feb 19, 2024
maxpatiiuk
commented
Feb 19, 2024
|
I also made an analogous change on the cc @acwhite211 |
CarolineDenis
approved these changes
Feb 19, 2024
Workaround for privatenumber/tsx#38 But also, it's time for those to be TypeScript Note, I added `// @ts-nocheck` to the top of the files to suppress TypeScript errors for now, but this should be removed once we move way from Backbone
I wasn't able to get swc-node to work. Tsx however works fine when using "type": "module" in package.json. And it's about time to convert to ESM anyway - while there would still be some friction expected, going forward, there would be more friction from not using ESM than using ESM
The update to node.js 20.11 seemed to cause the following issues to
appear. This commit resolves them:
- .css imports in node.js begun fail
- partially resolved by using esm-loader-css, but it is not
ready for production yet given
[the kind of bugs it has](brev/esm-loaders#10).
Also, it doesn't work correctly with swc-node and tsx (each tool
breaks in it's own way)
- thus, I instead wrote a small custom loader
- it doesn't work with non-default exports from JS files
- Converted to TypeScript in a3b51f7
- it seems to change the module resolution order(?) because a lot of
new errors regarding "window" being undefined started appearing - and
some of those are tricky to resolve (because Leflet crashes if it is
imported, even indirectly in the Node.js, and they
[refuse to fix it](Leaflet/Leaflet#6332)
saying it's not their responsibility)
- to be fair, running a small CLI command should not require importing
any React components or Leaflet, but our code is deeply
interconnected, and decoupling imports is tricky. And because of the
nature of an application like Specify 7, strict bundle separation
wasn't a priority, and changing that just for the sake of a few CLI
commands seems like a poor way to spend time
- Even after making all imports of Leaflet async, there is a problem
of `@shopify/draggable`, which references window in global scope
too, and who knows what other libraries there are that do that too
- Thus, I added JSDOM when running in node.js and excluded the
Leaflet plugin imports
- Pros: it works, and doesn't require refactor. It also executes a
lot of the codebase on any command invocation, making more errors
be caught before production
- Cons: it makes command invocation closer, but not by much, and
not a big deal as we don't have that many CLI commands
We don't need an `:is()` polyfill, so the warning is just an annoyance. Also, remove explicit `autoprefixer` as it's included in `postcss-preset-env`.
|
After spending several hours trying to resolve the build issues from https://github.com/specify/specify7/actions/runs/7963889013/job/21740345853, I got these results:
Outcome:
|
maxpatiiuk
changed the title
maxpatiiuk
commented
Feb 20, 2024
maxpatiiuk
commented
Feb 20, 2024
While we don't use .js anymore, our dependencies do
CarolineDenis
approved these changes
Feb 20, 2024
Triggered by 49d6920 on branch refs/heads/ci-refactor
melton-jason
approved these changes
Feb 21, 2024
There was a problem hiding this comment.
Glad to see this issue finally resolved!
To better ensure that the suggested changes (properly convert the old js files to ts and the REFACTOR suggestions with tailwind) are not lost and forever stuck as comments, the context can be put into an Issue (or two), if there is not another Issue which can capture the required context.
The Javascript files here are near their end! They've definitely overstayed their welcome 😄
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
and self-explanatory (or properly documented)
Testing instructions
Most changes in this PR are build-related, so don't affect the user-facing features, as long as everything builds correctly. Still, to be safe, please verify that map in query builder works correctly: