Skip to content
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

Support node 20 #96

Merged
merged 5 commits into from
May 7, 2023
Merged

Support node 20 #96

merged 5 commits into from
May 7, 2023

Conversation

giltayar
Copy link
Collaborator

@giltayar giltayar commented May 5, 2023

Fixes #95

Node.js 20 moved the loaders off the main thread, to their own thread. Usually this is not a problem for loaders, but quibble has an API that communicates with the loader itself. In versions prior to 20, this communication was done using global variables, but since the loaders are in a separate worker/thread, then this is not possible (workers/threads do not share global variables)

The only option for communication is to use messages between the main thread and the worker threads. Luckily, the loader team gave loaders the ability to do just that.

This PR enables the loader API to communicate with the loader using messages, if the loaders are off thread, and reverts to the previous method (global variables) if the loaders are in the main thread.

I took the liberty of removing crud related to ESM loaders in past versions. Also, the Github action now checks the current versions of Node, which are 16, 18, and 20.

I have also cleaned up the code and made Windows support easier by making all ESM code use URLs instead of file paths, because 1) that is the native language ESM speaks, 2) It is compatible with Windows, and 3) now that we can have HTTP (and other) urls as import specifiers, then we are not allowed to assume that the files are on disk!

@giltayar giltayar requested a review from searls May 5, 2023 14:33
@giltayar
Copy link
Collaborator Author

giltayar commented May 5, 2023

This also fixes testdouble/testdouble.js#513

@giltayar
Copy link
Collaborator Author

giltayar commented May 5, 2023

Hmm... Windows not working. Luckily, I have a Window machine now, so checking...

Copy link
Member

@searls searls left a comment

Choose a reason for hiding this comment

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

This is a big change! Looks like you modernized the style and APIs in use a bit with the refactor -- thank you!

lib/quibble.mjs Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
const result = await import('../esm-fixtures/a-module-with-function.mjs')
console.log('4')
Copy link
Member

Choose a reason for hiding this comment

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

Can probably clear these console logs when everything is working

@giltayar giltayar marked this pull request as draft May 6, 2023 05:26
@giltayar
Copy link
Collaborator Author

giltayar commented May 6, 2023

@searls sorry about the mess. The work on the Windows side (and a good night's sleep 😊 made me realize the code's design is problematic. I will probably spend today on some level of refactoring and get it to work correctly. So I'm guessing the above comments won't really have any meaning once I finish refactoring and making it work on both platforms, but I promise I will go over them once its done.

BTW, there are two main problems I see on the windows side:

  1. Windows/Unix paths vs file: URLs.
  2. It seems that during initialization, the order things happen is different between Windows and Linux.

Luckily I have windows machine with both WSL and Windows, so I can navigate back and forth between them easily.

@giltayar giltayar force-pushed the support-node-20 branch 4 times, most recently from ee09d20 to 7b0919e Compare May 6, 2023 20:03
@giltayar giltayar marked this pull request as ready for review May 6, 2023 20:07
@giltayar
Copy link
Collaborator Author

giltayar commented May 6, 2023

Major refactoring after it failed (spectacularly!) in Windows. The code is MUCH improved, and Windows compatibility should be much better going forward, because now all ESM code deals with URLs and not file paths (see more information about this in the PR description)

Ready for re-review!

@giltayar giltayar requested a review from searls May 6, 2023 20:09
module: await importFunctionsModule.importOriginalModule(fullImportPath)
// The name of this property _should_ be `moduleUrl`, but it is used in `testdouble` as `modulePath`
// and so can't be changed without breaking `testdouble`. So I add another field with the correct name
// and once testdouble is updated, I can remove the `modulePath` field.
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Let's update quibble and then peg TD to it so you can update it there too

test/esm-lib/quibble-esm.test.mjs Outdated Show resolved Hide resolved
test/esm-lib/quibble-esm.test.mjs Outdated Show resolved Hide resolved
test/esm-lib/quibble-esm.test.mjs Outdated Show resolved Hide resolved
@searls searls merged commit d9db27c into main May 7, 2023
6 checks passed
@searls
Copy link
Member

searls commented May 7, 2023

Released as quibble@0.7.0 and testdouble@3.18.0

@giltayar giltayar deleted the support-node-20 branch May 7, 2023 03:51
@DenyVeyten
Copy link

It looks like this PR drops node 14 support intentionally. I was trying to use quibble as a loader to mock esm dependencies for node 14-20, but now see that testdouble itself supports node 16+.
Any chance you can bring back node 14 support at least for quibble? :)

@searls
Copy link
Member

searls commented Nov 7, 2023

Node 14 is EOL since this April, and I can tell you from watching @giltayar work that getting ESM to function across 14, 16, 18, and 20 has required an incredible amount of patching and branching. Please expect this library to aggressively drop support for EOL'd versions so that we can keep the codebase simple and maintainable

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.

quibble does not work in Node.js 20 because loaders were moved off thread
3 participants