module: Unflag JSON modules

[aduh95]

JSON modules TC39 proposal has reached stage 3, this PR removes the experimental flag to use the feature. This PR doesn't implement https://github.com/tc39/proposal-import-assertions (which is still flagged in upstream V8); it should be backportable to LTS Release Lines.

//cc @nodejs/modules

Fixes: #37141

[devsnek]

i'd like to clarify with the v8 team if they will be providing an first class json module api before we unflag this.

[devsnek]

I think the environment option can be removed as well?

[aduh95]

Not on Node.js v15.x, v14.x, and v12.x. It can be removed in v16.x though.

[GeoffreyBooth]

I thought the proposal differed from what we offered behind our flag?

[devsnek]

@GeoffreyBooth they match the stage 3 proposal.

[bmeck]

I think we should only ship w/ the assertion required for now. There has been some disagreement on cache behavior on what we should do if concurrent requests w/ and without the assert occur.

[targos]

We won't be able to ship with assertion until V8 has a full API for it, and that will be with V8 9.0 (not before April 13). I think we can try to resolve the disagreement before that date?

[bmeck]

@targos we could yes. The key disagreement is having a cache with multiple entries during the concurrency which is what the web proposes vs coalescing to a single cache entry which is what some people want to do. I don't think we would have breakage if we shipped without resolving, but lack of resolution here might come back to bite us if internal leakage is visible is my main concern.

[devsnek]

@bmeck it might help if you provided a concrete example of that problem in a node.js program.

[bmeck]

@devsnek

given:

// make 2 parallel graphs loading
import('a');
import('a', {assert {type: 'json'}});

We can have 2 different requests here. 1 enforces type 1 does not. That means we have 2 different kind of ModuleJobs going on concurrently. Even if the asserted form fails it does not mean the one w/o an assert fails.

So, if they both resolve to the same URL we get a few things going on:

1. If the result is an ESM Source Text Module still need to differentiate the request so that only 1 succeeds.
2. If the result is a JSON Module still need to differentiate the request so that only 1 succeeds.

However, there is a potential that even if they resolve to the same location they could be given a different result when reading. The concern here is that in order to support the assert we have to differentiate them, and I can imagine both succeeding with 1 as Source Text Module and 1 as JSON.

This is potentially possible with loaders implementing HTTPS (including core shipping one), a location on disk being altered (/a -> /a.js in one fs op, and /a -> /a.json in the other), and I suspect that other scenarios are likely possible.

The solution to this kind of problem in the web is to error on mismatches and not coalesce during loading. That seems a fine solution to me but seems like it needs to have unique entries. I remain unconvinced it is impossible to replicate and have concerns as well for any future support for things like HTTPS. We can keep the cache entries separate and pointing to the same module if they match which seems fine to me, but there needs to be distinct rules on this behavior.

[devsnek]

@bmeck i think keeping the existing cache model and naively making the error in ResolveImportedModule if the type mismatches would work? the cache is idempotent and that logic is deterministic so you would get consistent behavior.

[bmeck]

@devsnek so you mean each module job would only have 1 type but be keyed on HREF and all subsequent requests would have to validate that type when finding a pending job before actual loading?

[devsnek]

@bmeck yes. in my mind the resolver cache has no assertions applied to it, those are handled more locally.