-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fix for NodeJS with ESM modules #10694
Conversation
Thanks for the pull request @ggetz!
Reviewers, don't forget to make sure that:
|
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.
This looks good to me, @ggetz. I just have a couple of things I wanted to clarify.
Thanks @sanjeetsuhag! This should be ready for another look. |
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.
Looks good. Thanks @ggetz!
Fixes #10684
NodeJS was failing to run ESM modules mainly because of a recent update to
zip.js
which requiresTransformStream
to be defined. Looks like the zip.js maintainer addressed this and suggests either locking to2.4.x
or adding a polyfill forTransformStream
. Here, I've locked the package version.To prevent related issues from going unnoticed in the future, I added a
.mjs
version oftest.cjs
, which is now run as part of CI.In doing so, I also caught two other issues. One was a faulty import, and the other was the use of
require
inResource.js
. If I understand correctly, this is code that should only ever be executed in a NodeJS environment, and can be replaced with a dynamic import , satisfying both the ESM and CJS use cases.