module: Unflag JSON modules #41736

GeoffreyBooth · 2022-01-29T00:05:15Z

This PR unflags JSON modules, removing the --experimental-json-modules flag. import of JSON files is already unflagged in Chrome, and the HTML spec has stabilized since the change that led to JSON modules’ unflagging being reverted two years ago in #29754. Deno also added support for the same syntax in denoland/deno#12866.

Since we added support for import assertions in #40250, I haven’t heard of any issues with the experimental implementation.

cc @MylesBorins @aduh95 @guybedford @nodejs/modules @nodejs/loaders

nodejs-github-bot · 2022-01-29T00:05:19Z

Review requested:

@nodejs/modules

lib/internal/modules/esm/translators.js

test/es-module/test-esm-non-js.mjs

ljharb · 2022-01-29T00:59:39Z

I still believe this should not happen until it's possible to import JSON modules without an assertion.

aduh95 · 2022-01-29T09:58:14Z

I still believe this should not happen until it's possible to import JSON modules without an assertion.

To clarify, you think the unflagging should wait, correct? Also, it is already possible to import JSON modules without an assertions using custom loaders, so would you be satisfied if there was a CLI flag to enable that?

JakobJingleheimer

Changes LGTM. I'm not concerned about the assertion-less scenario as it's already possible. I'm not aware of any other concerns that would prevent unflagging.

test/es-module/test-esm-non-js.js

aduh95

This section should also be updated:

node/doc/api/esm.md

Lines 480 to 494 in f458f1b

    
           #### No JSON Module Loading 
        
           JSON imports are still experimental and only supported via the 
        
           `--experimental-json-modules` flag. 
        
           Local JSON files can be loaded relative to `import.meta.url` with `fs` directly: 
        
           <!-- eslint-skip --> 
        
           ```js 
        
           import { readFile } from 'fs/promises'; 
        
           const json = JSON.parse(await readFile(new URL('./dat.json', import.meta.url))); 
        
           ``` 
        
           Alternatively `module.createRequire()` can be used.

Otherwise, LGTM 🤗

test/es-module/test-esm-invalid-data-urls.js

test/es-module/test-esm-non-js.js

doc/api/esm.md

ljharb · 2022-01-29T15:54:04Z

@aduh95 yes, i believe unflagging should wait, until it’s possible by default to import json assertionless, with no flags.

nodejs-github-bot · 2022-01-30T02:27:14Z

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

nodejs-github-bot · 2022-01-31T01:16:49Z

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

nodejs-github-bot · 2022-01-31T02:27:44Z

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

bmeck

I have no problems with this PR, but likely we should figure out how to describe cache behavior in a follow on for collisions due to the JS spec since there are multiple resolution strategies possible. I'm fine punting that effort due to scope being unclear.

aduh95 · 2022-01-31T18:52:11Z

@aduh95 yes, i believe unflagging should wait, until it’s possible by default to import json assertionless, with no flags.

@ljharb I'd also like to see assertionless JSON imports happen at some point (I even tried to make them happen), but I don't think it's reasonable to block the unflagging until it happens, and that was also the takeaway of the discussion last time it was brought up at the TSC meeting. Unflagging JSON modules now will let folks start exploring them in the wild, and gather more feedback. Holding it up wouldn't help much I'm afraid.

PR-URL: #41736 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

Notable changes: lib: * (SEMVER-MINOR) add fetch (Michaël Zasso) #41749 module: * unflag esm json modules (Geoffrey Booth) #41736 node-api: * (SEMVER-MINOR) add node_api_symbol_for() (Darshan Sen) #41329 stream: * (SEMVER-MINOR) add iterator helper find (linkgoron) #41849 * (SEMVER-MINOR) add toArray (Benjamin Gruenbaum) #41553 * (SEMVER-MINOR) add forEach method (Benjamin Gruenbaum) #41445 * (SEMVER-MINOR) support some and every (Benjamin Gruenbaum) #41573 deps: * upgrade npm to 8.4.1 (npm team) [#41836](#41836)

Notable changes: lib: * (SEMVER-MINOR) add fetch (Michaël Zasso) #41749 module: * unflag esm json modules (Geoffrey Booth) #41736 node-api: * (SEMVER-MINOR) add node_api_symbol_for() (Darshan Sen) #41329 stream: * (SEMVER-MINOR) add iterator helper find (linkgoron) #41849 * (SEMVER-MINOR) add toArray (Benjamin Gruenbaum) #41553 * (SEMVER-MINOR) add forEach method (Benjamin Gruenbaum) #41445 * (SEMVER-MINOR) support some and every (Benjamin Gruenbaum) #41573 deps: * upgrade npm to 8.4.1 (npm team) [#41836](#41836) PR-URL: #41897

PR-URL: #41736 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

Notable changes: * module: * unflag esm json modules (Geoffrey Booth) #41736 * doc: * add release key for Bryan English (Bryan English) #42102

Notable changes: * doc: * add release key for Bryan English (Bryan English) #42102 * module: * unflag esm json modules (Geoffrey Booth) #41736

danielleadams · 2022-03-03T15:39:07Z

@GeoffreyBooth just want to confirm for release purposes - should this be semver-minor or is this a patch?

ljharb · 2022-03-03T15:40:36Z

Unflagging is adding something new for the end user, so it definitely shouldn’t be a patch.

aduh95 · 2022-03-03T15:46:39Z

@GeoffreyBooth just want to confirm for release purposes - should this be semver-minor or is this a patch?

JSON modules are experimental, so semver rules don't apply to it. It does enable new capabilities that's true, maybe somone from @nodejs/lts have an option regarding this landing on a semver-patch LTS?

ljharb · 2022-03-03T15:53:26Z

To me that just means it can break in a non-major, not that semver loses all meaning.

danielleadams · 2022-03-03T16:10:33Z

fwiw, I think this should be a minor

GeoffreyBooth · 2022-03-03T17:24:22Z

This feels like a minor to me. It's arguably new functionality, not a bugfix.

danielleadams · 2022-03-03T17:35:48Z

Thanks all, i'll tag as minor

Notable changes: * doc: * add release key for Bryan English (Bryan English) #42102 * module: * unflag esm json modules (Geoffrey Booth) #41736

WebAssembly has been unflagged in V8 for years, and is presented on equal footing with Javascript in [how V8 describes itself][v8.dev] for years now ("Google’s open source high-performance JavaScript and WebAssembly engine"). I can't find any specified reasons to keep it flagged and called experimental in Node (though maybe this PR will draw those out?). Following nodejs#41736 as a model, this PR leaves the flag present as a no-op and removes it from documentation, but keeps the "Stability: 1 - Experimental" in esm.md and the runtime warning message. I want to remove those too (or else draw out some specified reasons not to :) ) but there appears to be a convention of decoupling unflagging from stabilizing: nodejs#41736 (comment), so, like JSON modules, maybe drop the warning in 18.0.0? [v8.dev]: https://v8.dev/

GeoffreyBooth added c++ module esm labels Jan 29, 2022

GeoffreyBooth requested a review from aduh95 Jan 29, 2022

nodejs-github-bot added the needs-ci label Jan 29, 2022

aduh95 reviewed Jan 29, 2022

View changes

lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved

test/es-module/test-esm-non-js.mjs Outdated Show resolved Hide resolved

aduh95 added the notable-change label Jan 29, 2022

GeoffreyBooth force-pushed the unflag-json-modules branch 2 times, most recently from e116204 to c63bfc3 Compare Jan 29, 2022

JakobJingleheimer approved these changes Jan 29, 2022

View changes

test/es-module/test-esm-non-js.js Show resolved Hide resolved

aduh95 approved these changes Jan 29, 2022

View changes

test/es-module/test-esm-invalid-data-urls.js Show resolved Hide resolved

test/es-module/test-esm-non-js.js Show resolved Hide resolved

guybedford approved these changes Jan 29, 2022

View changes

robpalme approved these changes Jan 29, 2022

View changes

doc/api/esm.md Outdated Show resolved Hide resolved

VoltrexMaster approved these changes Jan 29, 2022

View changes

GeoffreyBooth force-pushed the unflag-json-modules branch from c63bfc3 to bd1f35b Compare Jan 30, 2022

GeoffreyBooth force-pushed the unflag-json-modules branch 2 times, most recently from a85b524 to a63df21 Compare Jan 31, 2022

GeoffreyBooth mentioned this pull request Jan 31, 2022

Https imports #36328

Merged

9 tasks

GeoffreyBooth added author ready and removed needs-ci labels Jan 31, 2022

targos approved these changes Jan 31, 2022

View changes

bmeck approved these changes Jan 31, 2022

View changes

github-actions bot mentioned this pull request Feb 4, 2022

CI Reliability 2022-02-04 nodejs/reliability#192

Open

38 tasks

github-actions bot mentioned this pull request Feb 5, 2022

CI Reliability 2022-02-05 nodejs/reliability#193

Open

35 tasks

ruyadorno mentioned this pull request Feb 8, 2022

v17.5.0 release proposal #41897

Merged

weswigham mentioned this pull request Feb 9, 2022

Remove the error on importing a json document in esm files in moduleResolution: nodenext microsoft/TypeScript#47819

Open

silverwind mentioned this pull request Feb 10, 2022

Rule proposal: prefer-json-parse-buffer sindresorhus/eslint-plugin-unicorn#1273

Closed

aduh95 mentioned this pull request Feb 19, 2022

module: Unflag JSON modules #37375

Closed

danielleadams added a commit that referenced this issue Mar 3, 2022

2022-03-08, Version 16.14.1 Gallium (LTS)

4446216

Notable changes: * module: * unflag esm json modules (Geoffrey Booth) #41736 * doc: * add release key for Bryan English (Bryan English) #42102

danielleadams mentioned this pull request Mar 3, 2022

v16.14.1 release proposal #42200

Merged

danielleadams added a commit that referenced this issue Mar 3, 2022

2022-03-08, Version 16.14.1 Gallium (LTS)

1fd4428

Notable changes: * doc: * add release key for Bryan English (Bryan English) #42102 * module: * unflag esm json modules (Geoffrey Booth) #41736

danielleadams added the semver-minor label Mar 3, 2022

danielleadams added a commit that referenced this issue Mar 3, 2022

2022-03-08, Version 16.14.1 Gallium (LTS)

d91e132

Notable changes: * doc: * add release key for Bryan English (Bryan English) #42102 * module: * unflag esm json modules (Geoffrey Booth) #41736

broofa mentioned this pull request Mar 8, 2022

node@16.14 import assertions break examples/node-esmodules/example.mjs test uuidjs/uuid#619

Closed

gthb mentioned this pull request Mar 12, 2022

module: unflag WASM modules #42309

Open

nodejs / node Public

module: Unflag JSON modules #41736

module: Unflag JSON modules #41736

GeoffreyBooth commented Jan 29, 2022 •

edited

nodejs-github-bot commented Jan 29, 2022

ljharb commented Jan 29, 2022

aduh95 commented Jan 29, 2022

JakobJingleheimer left a comment

aduh95 left a comment

ljharb commented Jan 29, 2022 •

edited

nodejs-github-bot commented Jan 30, 2022

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Jan 31, 2022

bmeck left a comment

aduh95 commented Jan 31, 2022

danielleadams commented Mar 3, 2022

ljharb commented Mar 3, 2022

aduh95 commented Mar 3, 2022 •

edited

ljharb commented Mar 3, 2022

danielleadams commented Mar 3, 2022

GeoffreyBooth commented Mar 3, 2022

danielleadams commented Mar 3, 2022

	#### No JSON Module Loading

	JSON imports are still experimental and only supported via the
	`--experimental-json-modules` flag.

	Local JSON files can be loaded relative to `import.meta.url` with `fs` directly:

	<!-- eslint-skip -->

	```js
	import { readFile } from 'fs/promises';
	const json = JSON.parse(await readFile(new URL('./dat.json', import.meta.url)));
	```

	Alternatively `module.createRequire()` can be used.

nodejs / node Public

module: Unflag JSON modules #41736

module: Unflag JSON modules #41736

Conversation

GeoffreyBooth commented Jan 29, 2022 • edited

nodejs-github-bot commented Jan 29, 2022

ljharb commented Jan 29, 2022

aduh95 commented Jan 29, 2022

JakobJingleheimer left a comment

aduh95 left a comment

ljharb commented Jan 29, 2022 • edited

nodejs-github-bot commented Jan 30, 2022

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Jan 31, 2022

bmeck left a comment

aduh95 commented Jan 31, 2022

danielleadams commented Mar 3, 2022

ljharb commented Mar 3, 2022

aduh95 commented Mar 3, 2022 • edited

ljharb commented Mar 3, 2022

danielleadams commented Mar 3, 2022

GeoffreyBooth commented Mar 3, 2022

danielleadams commented Mar 3, 2022

GeoffreyBooth commented Jan 29, 2022 •

edited

ljharb commented Jan 29, 2022 •

edited

aduh95 commented Mar 3, 2022 •

edited