Repository navigation
The future of --experimental-json-modules #37141
Description
Activity
JSON Modules will be stage 3 once reviewers have finished reviewing it.
Reacted by Michaël Zasso, Myles Borins, Derek Lewis and LinzI would pull the experimental json modules and use the official v8 impl when it lands.
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.
@zackschuster importing json does not require
type: json, but any import withtype: jsonmust be a json module.Reacted by Zack Schuster and Matthew Phillips@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 :)
@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.
Reacted by Zack Schuster and Niklas Mischkulnig@bmeck would that just mean another read from disk, or is it supposed to be possible to observably mutate the json module?
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)
Assertions are specifically part of resolution, see tc39/proposal-json-modules#10
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.
@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.
Reacted by Zack SchusterThe 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.
7 remaining items
@devsnek kind of? If the resolution is to a path
/foo.jsonany it loads normally you get an entry/foo.jsonin the global module map presumably matching/foo.jsonandtype:json(optional?). Then if it is loaded w/ a differenttypesuch as missing the type it could instead on disk show that it is meant to load/bar.jsafter the 2nd resolve. This would no longer match thetype:jsonduring a resolve. The HTML approach is a 2 tiered cache where you check for that resolved path/foo.jsonand 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.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"?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`); } }
Might need some bikeshedding but i like the separation 👍
JSON Modules are now stage 3: tc39/proposals@eda6da4
Reacted by Jordan HarbandI think we can close this now that we have a path forward; @targos please reopen if you feel otherwise.
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-moduleswith it? Should we add a new experimental flag? Should we deprecate the current behavior of--experimental-json-modules?@nodejs/modules