GeezerVison

cc @jasnell

@szmarczak is that c4f0058 that you expect?

cc @nodejs/modules

I don't think this is necessarily something Modules needs to do anything with, it looks like it is just because import is doing async work that it is showing up? It does look weird though and IDK if this is the right fix since it looks like the close() operation is happening to close both streams before the 2nd gets the data buffered? Not familiar w/ this streams impl but it doesn't seem to be specific to ESM itself.

The added test doesn't fail before the fix. -- @targos

You're right, I didn't notice it before the changes. I was using a .mjs to validate locally. Fixing it soon.

if this is the right fix since it looks like the close() operation is happening to close both streams before the 2nd gets the data buffered? -- @bmeck

I felt the same when looking at it, looks like a conditional flag before closing the stream is needed. But, I don't have enough knowledge in the streams/web to assume that.

@targos Just created a new test with fails without this modification (.mjs)

I'll have to stew over this change a bit. Changing the timing on the close using nextTick just doesn't feel right and changes the timing guarantees from what is spec'd. But, I need to think through about whether that's going to be a problem or not. I'm not giving this a thumbs down, just need to think about it more.

changes the timing guarantees from what is spec’d

I’m also wondering if this is a breaking change.

Also how does equivalent code behave in browsers? This seems like it’s essentially a spec question, as in are we following the spec-defined behavior. If browsers behave differently then Node does now (and like how this PR changes Node to behave) that would be a strong argument for making the change, because it would imply that Node’s implementation is incorrect.

Please don't forget to label with streams and ping @nodejs/streams on these kind of PRs.

@jasnell Have you considered the following example? #39758 (comment)

CI: https://ci.nodejs.org/job/node-test-pull-request/41516/

@targos can you take a look again?

CI: https://ci.nodejs.org/job/node-test-pull-request/41564/

CI: https://ci.nodejs.org/job/node-test-pull-request/41565/

Landed in 113133b