search

standard-things / esm

Tomorrow's ECMAScript modules today!
Other
5.26k stars 146 forks source link

esm is broken by change in Node.js #821

Open targos opened 5 years ago

targos commented 5 years ago

Found the breakage with CITGM while preparing Node.js 12.5.0.

From this PR: https://github.com/nodejs/node/pull/21387 Affected code: https://github.com/standard-things/esm/blob/6feb50b1e7366f1ed075ed5ce05c8832e5493278/src/fs/stat-fast.js#L9

The isFile method is no longer directly on Stats.prototype but on StatsBase.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 of safe/fs.js does not handle this correctly.

jdalton commented 5 years ago

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?

targos commented 5 years ago

@jdalton You can see the failing modules here: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1878/nodes=fedora-latest-x64/ citgm.clinic-v4.1.0 citgm.fastify-v2.5.0 citgm.semver-v6.1.1 citgm.split2-v3.1.1

mcollina commented 5 years ago

It's the https://www.node-tap.org/ test framework that is breaking because of this.

targos commented 5 years ago

Is there a path to fix this? I don't want to hold https://github.com/nodejs/node/pull/21387 out of v12.x for too long.

jdalton commented 5 years ago

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 https://github.com/standard-things/esm/commit/4f35a2407681717d1848a7764eaa02f772f7e65a.

BridgeAR commented 5 years ago

@jdalton thanks a lot for looking into this!

mcollina commented 5 years ago

@jdalton any updates on this? Thanks!

jdalton commented 5 years ago

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.

mcollina commented 5 years ago

https://github.com/mcollina/split2 tests are failing on master because tap is failing to load files.

targos commented 5 years ago

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.

Trott commented 5 years ago

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?

BridgeAR commented 5 years ago

@jdalton is there any further update?

gtarsia commented 4 years ago

Considering this and the other issues with node 12, should pkgs using esm recommend using node < 12? EDIT: I guess I misread the issues, disregard this.

Trott commented 4 years ago

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".

Trott commented 4 years ago

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: https://github.com/nodejs/node/pull/28957 Refs: https://github.com/nodejs/node/commit/6ff803d97c8849571bb95f46ba6520150b78ccd3