The future of --experimental-json-modules

[targos]

The import assertions proposal is in stage 3 and V8 implemented it (the API will be available in V8 8.9).
The JSON modules proposal is in stage 2 and is built on top of import assertions.

I think it's time to discuss about what we are going to do with --experimental-json-modules.
We'll be able to implement the type: "json" assertion soon. Should we aim to replace --experimental-json-modules with it? Should we add a new experimental flag? Should we deprecate the current behavior of --experimental-json-modules?

@nodejs/modules

[ljharb]

JSON Modules will be stage 3 once reviewers have finished reviewing it.

[devsnek]

I would pull the experimental json modules and use the official v8 impl when it lands.

[zackschuster]

assuming assertions are mandatory, i don't think the flag should "just" be pulled, for the sake of those unable to adopt new syntax (e.g. teams stuck on an older typescript by angular 8). they'd be effectively blocked from updating the platform if they ever relied on importing json. i'd recommend replacing the flag instead with one that allows omitting assertions.

[devsnek]

@zackschuster importing json does not require type: json, but any import with type: json must be a json module.

[zackschuster]

@devsnek that's good then. last i heard was that it wasn't certain if the assertion would be mandatory, at least under node. being able to simply drop the flag without code changes is nice :)

[bmeck]

@zackschuster likely it won't be mandatory under node, but due to some reasons likely it won't have the same cache key between them so you could get 2 different modules if you do or do not include the assertion.

[zackschuster]

@bmeck would that just mean another read from disk, or is it supposed to be possible to observably mutate the json module?

[devsnek]

in the context of node's resolver I would not expect it to use type:json as part of the cache key (assertions are specificallynot resolver attributes)

[bmeck]

Assertions are specifically part of resolution, see tc39/proposal-json-modules#10

[ljharb]

They are allowed to be; they are also allowed not to be, and encouraged not to be. The web won’t make them part of the cache key when it can avoid it, for example - which won’t be always.

[bmeck]

@zackschuster I'm unclear on the question, I would expect if we follow the web spec, it usually wouldn't cause a secondary read due to having a fetch cache (node currently does not have one). In the web if a HREF resolves and it has an existing entry w/ a different type an error is expected to be thrown, I don't think Node would throw that error. So, we could see 2 disk reads if there isn't a cache, same as today; and JSON modules are mutable so mutation is observable.

[bmeck]

The web won’t make them part of the cache key when it can avoid it, for example - which won’t be always.

This is 100% false, in the web they will 100% of the time be part of the cache key when they go through the fetch infrastructure.

[devsnek]

@bmeck my understanding and consensus was on them not being required to be part of the cache key. is that not the case?

[ljharb]

@bmeck but the cache will be normalized, which means that effectively they will not be part of the cache key the majority of the time, which means the web will effectively be following the recommendation to use the stronger guarantee.

[bmeck]

@devsnek it is complicated, but at least if things want to have compatibility they need to be part of the cache key and then error when collisions occur. This largely affects parallel graphs or race conditions due to IO.

[bmeck]

@ljharb they are not following the recommendation intentionally per the HTML integration PR.

[devsnek]

@bmeck assertions don't change how something is loaded, I'm not sure how that would cause a race or collision.

[targos]

@devsnek that's entirely up to us. The V8 API will give us the assertions along with the specifier and we can decide to change how the module is loaded using them.

[bmeck]

@devsnek In Node for example if you have a resolve operation that points to /foo.json but the actual fetch ends up at /bar.js due to insertion of some kind of FS aliasing mechanism occurring between resolve and fetch like symlinks you get into races. Per HTML this also occurs when you have servers giving differing content-types. The point of the HTML integration was to have the cache use assertions to prevent churn and then error when collisions are seen. I can imagine us not throwing on collision, but lack of it in the cache key seems unwise since it would be a strange divergence and one that we can't exactly match unless we treat loading as an atomic operation.

[devsnek]

@targos from the proposal:

Implementations are not permitted to interpret a module differently at multiple import sites if the only difference between the sites is the set of import assertions. Future follow-up proposals may relax this restriction with "evaluator attributes" that would change the contents of the module.

We could do weird things of course, but I'm not sure why we would want to.

[bmeck]

@devsnek that is interpretation not evaluation.

[devsnek]

@bmeck I'm not following your example about aliasing. an assertion is a local requirement on the resolved module.

[bmeck]

@devsnek kind of? If the resolution is to a path /foo.json any it loads normally you get an entry /foo.json in the global module map presumably matching /foo.json and type:json(optional?). Then if it is loaded w/ a different type such as missing the type it could instead on disk show that it is meant to load /bar.js after the 2nd resolve. This would no longer match the type:json during a resolve. The HTML approach is a 2 tiered cache where you check for that resolved path /foo.json and would see a map of types that already exist for the HREF and then error if one already exists and isn't what we resolve. So, it is still part of the cache key, but avoids collisions. We could remove the error part of that algorithm (similar to what we did with bare imports), but having it resolve still would now need to deal with situations like above.

[devsnek]

so in my head canon, our resolver should look like this: (specifier, referrer) -> all the scary resolution and hooks and stuff -> module -> verify assertions. Is this what you meant by being "atomic"?

[bmeck]

@devsnek by "atomic" I mean that no side effects (such as disk manipulation) can occur during that chain.

[devsnek]

@bmeck if assertions aren't part of the cache key, the addition or removal of them won't cause the resolver to hit the disk again, so it seems like a moot point?

[bmeck]

@devsnek the resolver always hits the disk to resolve new specifiers (if they are not simple URL manip) so it should hit disk for a and then b as specifiers and both can resolve to the same location.

[devsnek]

@bmeck i'm still not following.

[Jamesernator]

Regarding the loader, given the intention is purely to assert conditions about the resource, it would make sense to add this as an additional loader hook (that returns nothing, but might throw an error) e.g.:

const isYaml = Symbol('isYaml');

export default async function load(context, load) {
  const { url, format, content } = await load(context);
  if (format === 'application/yaml' || format === undefined && url.endsWith(".yaml")) {
    context[isYaml] = true;
    // parse yaml and transform into JSON...
    const jsonContent = JSON.stringify(...);
    return { format: 'json', content: jsonContent };
  }
  return { format, content };
}

export default function validateAssertions(
  context,
  defaultValidateAssertions,
) {
  const { assertions, format } = context;
  if (assertions.type === 'yaml' && !context[isYaml]) {
    throw new Error(`Expected yaml but got ${ format } instead`);
  }
}

[devsnek]

Might need some bikeshedding but i like the separation 👍

[aduh95]

JSON Modules are now stage 3: tc39/proposals@eda6da4

[GeoffreyBooth]

I think we can close this now that we have a path forward; @targos please reopen if you feel otherwise.