-
Notifications
You must be signed in to change notification settings - Fork 146
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
esm is broken by change in Node.js #821
Comments
Ah, thank you @targos for the ping. I'll dig into this. I'll try to release a patch ASAP after finding the root cause. Out of curiosity which project is being checked by CITGM? |
@jdalton You can see the failing modules here: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1878/nodes=fedora-latest-x64/ |
It's the https://www.node-tap.org/ test framework that is breaking because of this. |
Is there a path to fix this? I don't want to hold nodejs/node#21387 out of v12.x for too long. |
Hi @targos! Yes, there's totally a path to fix this. I'll try to tackle it within a day (hold me to hit) and I'll keep you posted. Update: I got one set of failing tests to pass. Now wrapping up new failing test262 tests. Update: Got other set of failing test262 tests to pass. On to the actual issue now :) Update: Patched to potentially resolve issue 4f35a24. |
@jdalton thanks a lot for looking into this! |
@jdalton any updates on this? Thanks! |
I've patched this locally but would like a way to test this. Can you point me to the Node PR that is effected so I can build locally and test and/or suggest a unit test that may cover this scenario. |
https://github.com/mcollina/split2 tests are failing on master because tap is failing to load files. |
Another way to test this is just to run this repo's tests with Node.js master (you can install a nightly build for example). They failed the same way tap does last time I tried. |
Polite/gentle ping! (Would be great to get this resolved so we can more easily test breaking changes in Node.js.) Anything I or someone else can do to make things easier and nudge this closer to the finish line? |
@jdalton is there any further update? |
Fix to unblock CITGM. See, standard-things/esm#821. PR-URL: nodejs#28957 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Fix to unblock CITGM. See, standard-things/esm#821. PR-URL: #28957 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Considering this and the other issues with node 12, should pkgs using esm recommend using node < 12? |
This issue has been worked around since Node.js 12.10.0 so unless you can specify what you mean by "the other issues", I'd say the answer is probably "no". |
Refs: nodejs/node#28957 |
Found the breakage with CITGM while preparing Node.js 12.5.0.
From this PR: nodejs/node#21387
Affected code:
esm/src/fs/stat-fast.js
Line 9 in 6feb50b
The
isFile
method is no longer directly onStats.prototype
but onStatsBase.prototype
instead.The code should theoretically still work because the prototype chain is setup with
Object.setPrototypeOf(Stats.prototype, StatsBase.prototype);
. My guess is that it's broken because the setup ofsafe/fs.js
does not handle this correctly.The text was updated successfully, but these errors were encountered: