Revert "module: disable cjs snapshotting into esm loader"

[devsnek]

This reverts commit 1fe39f0. It was landed because I did not clearly mark my objection to it.

Refs #34467

[nodejs-github-bot]

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

[guybedford]

@devsnek can you please clearly state your objection here? Let's discuss this at the next modules meeting if you're able to join.

[guybedford]

Awaiting an expanation of the reason for this PR.

[devsnek]

@guybedford like i said in the other thread: "i think the snapshots are an important behavior we should keep."

[guybedford]

With all due respect @devsnek that is simply not an adequate response for this, my block remains.

[devsnek]

????????????? @nodejs/tsc

[guybedford]

@devsnek there are pros and cons at stake here, and a justification for this PR needs to be based upon an argument that the benefits of snapshotting outweigh the issues that we have been discussing the last few weeks including the non GC-ability of all CommonJS modules as well as the inability for loaders to support named exports, which is exactly needed because we are working around your concerns with regards to the current named exports proposal.

[devsnek]

@guybedford it is my informed opinion that the cons outweigh the pros and it is my right as a collaborator to block because of that. I'm sorry that I wasn't clear enough about it in the original PR, and I realize that this is frustrating, but it should be reverted until we reach consensus.

[guybedford]

@devsnek we seem to be discussing process more than the underlying technical issues at stake which is a little concerning to me. The PR landed after due time and with discussion in the modules meeting. I am sorry if you did not see it, and am happy to discuss the technical concerns, but landing this PR without a careful discussion of those concerns by default is not the process as far as I'm aware of it. Certainly let's seek TSC guidance further as necessary.

[guybedford]

To reiterate - my block is primarily that there is no clear justification being provided for this revert apart from that personal preference.

[devsnek]

if you want to discuss process, as far as I'm aware pressing the red button is not required to object, and I did state I didn't feel comfortable with the changes. From my perspective it seems like you tried to slip the pr through on a technicality, but I'd rather not discuss that because I don't think it's constructive.

[guybedford]

I'm sorry you felt that way about the PR - I would not have landed it if I had known of your objection at all, but there was no indication to me that you were objecting to it. In addition we would have discussed your objection in the meeting in that case. I must admit I am still not clear on what exactly your objection is - it would help a lot if you could try to put it into words further for me.

I've added the TSC agenda label as it does sound like it hits up against some process concerns.

At the same time I really hope we can get back to ernestly discussing the underlying details of the technicalities here with regards to what is best for the project.

[MylesBorins]

howdy. Chiming in with my TSC hat on here.

I think the appropriate thing to do here is fast track this reversion, reopen the original PR, and attempt to work through the objections there. If we can't work through those then we should escalate to the TSC per policy.

Can folks emoji respond to this comment to show support? @guybedford would you be willing to drop your objection?

We have a TSC call today where we can discuss this issue / objection and come back to the PR with some thoughts... although most likely we are not going to give much better advice than "please attempt to work through the objections in the PR".

To me the important thing here is that we respect the fact that an objection was intended and move the discussion about that objection to the PR about landing this change.

[guybedford]

I was not aware a retrospective rejection was possible, so this is news to me personally. Is this really a process we want to encourage for the project? I appreciate that "sneaking things through unnoticed" could be a concern, but this was discussed publicly in the modules meeting. It is also worth noting that Gus isn't currently attending these meetings at all, which makes finding consensus very difficult without the communication. Specifically - I have concerns over the modules process over how powerful his objections can be without participating in the discussions, we're already spending lots of time in the meetings trying to infer his objections, and having to make assumptions about them.

Personally I am not sure I have the energy to fight with this process in seeking a synthesis on such a hard problem, when as far as I'm concerned landing this PR was a useful step for users, and would likely otherwise let the status quo lie, which seems like a worse outcome for the project overall.

[MylesBorins]

@guybedford I'll bring this up in the TSC meeting today and come back with a recommendation on how we should move forward here

[mmarchini]

@MylesBorins the TSC meeting was canceled yesterday due to lack of agenda items, so we won't be discussing it today (unless @mhdawson re-instates the meeting).

[mmarchini]

Also with my TSC hat on:

Our policy states:

Where there is disagreement among Collaborators, consensus should be sought if possible. If reaching consensus is not possible, a Collaborator may escalate the issue to the TSC.

Collaborators should not block a pull request without providing a reason. Another Collaborator may ask an objecting Collaborator to explain their objection. If the objector is unresponsive, another Collaborator may dismiss the objection.

I agree with @MylesBorins, there was an objection and it should be discussed with @devsnek in the original PR, therefore I'm in favor or reverting this PR. I do ask @devsnek (without blocking this revert) to elaborate their objection though so we can have a more productive discussion: why is it an important behavior for us to keep? Which use cases does it provide? Or is it more of a performance concern?

[guybedford]

Thanks for the TSC input here @mmarchini @MylesBorins. I am happy to concede if the consensus is that clarifying a block can be done after a merge, but as I say that is not clear to me here. Note that my block here is not actually about the revert itself, but that the objection is not being clarified beyond "this feature is needed" which gives nothing to work with in terms of finding any middle ground and basically means dropping the approach entirely otherwise.

[devsnek]

@guybedford I'm happy to discuss my concerns further (elsewhere). No one asked me to clarify my comment on the original pr so I assumed it was understood.

[guybedford]

Ok understood @devsnek, in that case, in the name of not holding things up for one week for the TSC meeting, then I am fine with merging this PR, on the condition that the objection is fully clarified beyond the statement that "snapshotting is needed" either in this issue or on the original PR.

[guybedford]

Ok thanks @devsnek for the comment in the other thread, that seems like a good start to me.

So require('m') will get the value of module.exports at the time of the require, while import('m') for a CommonJS module will get the value of module.exports from the very first moment that CommonJS module was loaded whenever that might have been, for all CommonJS modules. That is fine but as mentioned there is a GC issue with this locking all CommonJS modules into the ESM caches in a way that can never be flushed and in applications that unload modules from require.cache making it impossible to have those modules be GC'd.

It sounds to me then like the GC issue might be tackled by some out of band CJS cache flush process then. I will leave it to others to work out the details here if/when the need arises. I would suggest anyone interested to work on this before 12 LTS ends if this is deemed important.

[mmarchini]

I am happy to concede if the consensus is that clarifying a block can be done after a merge, but as I say that is not clear to me here.

Just to clarify so we don't create a (potentially dangerous) precedence: I don't agree with that statement, @devsnek made an objection and there was no attempt to seek consensus with them or to clarify the objection in the PR (both with are required in our governance documents), therefore the PR was merged with an objection. The objection still existed, even if the author didn't interpret it as an objection or didn't think the objection was clear.

Misunderstandings happen and we should discuss how to improve the process so these things are less likely to happen in the future. Feel free to chime in on #34564 if you have suggestions.