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.

[bmeck]

@devsnek how would that handle (bear with my usage of a theoretical type):

// make 2 parallel graphs loading
import('a');
//
// some time later, but before done w/ the module job above
//
import('a', {assert {type: 'json'}});
import('a', {assert {type: 'javascript'}});

The pending module job would match both of the types 'json' and 'javascript' if it starts with an unchecked type.

[ljharb]

Assuming the actual type is "json" or "javascript", and assuming both are optional, i'd assume that that would result in (via forced sync cache interaction) 2 successful requests with the same Module record, and one rejected request.

[devsnek]

@bmeck the first eventually fulfills, one of the other two eventually fulfills and the other eventually rejects.

[aduh95]

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

Do you know if there is a time frame for this discussion, and where we can follow updates?

[devsnek]

we should be good to go. it doesn't look like they'll be providing a wrapper.

[aduh95]

we should be good to go. it doesn't look like they'll be providing a wrapper.

Great! Are you still -1 for this PR then?

[GeoffreyBooth]

Just so that I’m clear, are browsers going to allow import('./file.json'), or only import('./file.json', {assert {type: 'json'}})?

[bmeck]

@GeoffreyBooth only with the assert

[aduh95]

@bmeck the Import Assertion proposal README seems to indicate caching won't be a problem: https://github.com/tc39/proposal-import-assertions#how-would-this-proposal-work-with-caching

How would this proposal work with caching?

Assertions are not part of the module cache key. Implementations are required to return the same module, or an error, regardless of the assertions.

Does that address your concern, or are you talking about something else?

[ljharb]

@aduh95 that part of the proposal readme is a bit out of date; it's a weird hybrid scenario where it shouldn't be part of the cache key, but due to the browser's inability to control servers doing weird things (for example, if a server chose not to serve modules idempotently), the spec had to be loosened to permit them to be part of the cache key. In the ideal scenario (and the common case) it will act as if they were not part of it, but this will only come up in scenarios where the assertion is optional - which 'json' in browsers is not.

[aduh95]

it does a disservice to our users and the ecosystem for Node to allow a syntax that browsers disallow

Let's imagine you have a JS module that contains:

import packageConfig from './package.json';

Can this syntax be used in the browser? Well maybe, we can't tell, because using a .json extension does not mean that the returned module would be of type JSON; so I don't think we can say the browsers disallow this syntax, it has a different meaning, granted, but as far as the web server is configured in this way, it is universal code.

Just because JavaScript doesn’t require assertions doesn’t mean that we should allow assertionless syntax for other file types.

Well that was my point exactly: since the web can always convert all modules to JS, why should Node.js requires the assertion?

That encourages people to write packages that run only in Node

What about other Node.js APIs? Why would assertionless JSON import be different?

[GeoffreyBooth]

I don’t have endless time to debate this. I don’t find much, if any, value in supporting the assertionless syntax. The assertion-required syntax satisfies the use case and is compatible with browsers, so let’s add it and ship it. We don’t need anything else.

[ljharb]

A number of people are telling you that they (we) do want it and need it - those concerns are all invalidated just because you decided that we don't actually need it?

There hasn't been any debate yet - all you've said, albeit multiple times, is "browsers don't have this, so node shouldn't either" - which seems to be based on a presumption that node should only have things browsers do. However, this is already not the case, because node is not a browser, and will continue not to be the case in the future, because node has vastly different use cases and requirements and demographics than browsers do.

[Jamesernator]

A number of people are telling you that they (we) do want it and need it - those concerns are all invalidated just because you decided that we don't actually need it?

I for one have seen a lot of people saying they want it yes, but it's not clear to me that it is strictly need-ed. Ultimately Node has to support the assertion form in order if it wants to implement JSON modules at all to be spec compliant (with the JSON modules spec), having an assertion-free version is not needed for spec compliance.

There hasn't been any debate yet - all you've said, albeit multiple times, is "browsers don't have this, so node shouldn't either" - which seems to be based on a presumption that node should only have things browsers do. However, this is already not the case, because node is not a browser, and will continue not to be the case in the future, because node has vastly different use cases and requirements and demographics than browsers do.

Yes, but it also ships by default with the biggest JS repository (npm) that is used to distribute code for multiple environments, many many packages may write their tests using a Node test runner but otherwise not depend on Node.

How is it different from import { namedImport } from './module.cjs'? It's not supported by browsers either, and probably never will. The ecosystem have tools to work around this limitation (either by web server configuration, or by bundling), most of them already exist because require('./file.json') is already a thing anyway, so why would assertionless JSON modules be an issue?

I don't think the cost of tooling should be underestimated, tooling has various costs, I think the biggest is actually reliability and consistency of behaviour. I don't think I've ever had a case where I haven't had to work around bugs or quirks in tooling, sometimes these costs are small, but sometimes they can be incredibly difficult to identify and resolve.

---

I'm not a person who can do so, but personally if I were able to I would definitely block shipping assertion-less import as well, simply for the reason that --experimental-specifier-resolution is a thing. By shipping assertion-less imports by default, but not Node specifier resolution leads to a bizarre inconsistency where some parts of module importing is inconsistent with browsers, but some of it isn't.

If Node does want to ship assertion-less imports by default then it should really be shipping --experimental-specifier-resolution=node as the default as well. The reasons people want both of these features is very similar, having a weird inbetween where some features are shipped, but not others, for the same use case doesn't seem like a good approach to me at all.

Really Node should be looking at the use cases for assertionless imports, --experimental-specifier-resolution, and any other features with similar use cases and decide if they want to actually support those use cases or not. If they do they should be ensuring all of those features and work together in a consistent and expected way.

[GeoffreyBooth]

Converting to draft for now, as this can’t land soon. Per TSC meeting, #40250 needs to land first (with only the assertion-required syntax, only for JSON) and that needs to ship and be in the wild a bit so we can gather feedback. Once adequate time has passed and any issues that users report are addressed, JSON modules should be good to unflag.

[aduh95]

Superseded by #41736