Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: Revert remove experimental status from JSON modules #29754

Closed
wants to merge 4 commits into from

Conversation

@guybedford
Copy link
Contributor

guybedford commented Sep 28, 2019

This reverts commit ec8776d from #27752, adding back the --experimental-json-modules flag to reflect the fact that JSON modules are being reverted for web (whatwg/html#4943) and should not be considered a stable feature.

Tracking issue at nodejs/modules#391 with further information.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Copy link
Member

devsnek left a comment

the web's issue doesn't match our security model, I don't think we need to destabilise this

@addaleax addaleax added the ES Modules label Sep 28, 2019
@littledan

This comment has been minimized.

Copy link

littledan commented Sep 29, 2019

I think it's possible that we will eventually want to use exactly this syntax for importing JSON modules in Node.js long term, for the reasons @devsnek says. However, the sad state is, we just don't know what the Web will do at this point--it is also possible that the Web may include some changes that we end up wanting to do in Node as well. So the conservative option would be to mark this as experimental again.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Sep 29, 2019

we already do this differently than the web (with the current system you have to explicitly type in .json in your import), so i don't think there was ever any way to converge this in the first place.

@littledan

This comment has been minimized.

Copy link

littledan commented Sep 29, 2019

Yes, I agree it's possible that it will never converge. But we haven't really done the design work on the web side so I can't personally draw a conclusion with confidence.

@nodejs-github-bot

This comment has been minimized.

@@ -310,6 +315,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
AddOption("--experimental-exports",
"experimental support for exports in package.json",
&EnvironmentOptions::experimental_exports,

This comment has been minimized.

Copy link
@Trott

Trott Oct 1, 2019

Member

It appears that this isn't valid C++ and will not compile.

EDIT: Well, this line is the start of a valid statement, but then the next few lines....you probably get what I mean. This:

20:46:37 ../src/node_options.cc: In constructor ‘node::options_parser::EnvironmentOptionsParser::EnvironmentOptionsParser()’:
20:46:37 ../src/node_options.cc:321:35: error: expected ‘)’ before ‘;’ token
20:46:37              kAllowedInEnvironment);
20:46:37      
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 1, 2019

I'm in favor of making it experimental personally on "when in doubt, keep it experimental" grounds, but I would happily defer to Modules WG if there's consensus. Any chance this goes on the Modules WG agenda or something to come to a resolution?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 1, 2019

Added the "work in progress" label. Feel free to remove once it compiles.

@guybedford guybedford force-pushed the guybedford:json-modules-flag branch 4 times, most recently from 39e2f6b to 683f429 Oct 1, 2019
@devsnek devsnek dismissed their stale review Oct 1, 2019

@guybedford guybedford force-pushed the guybedford:json-modules-flag branch from 683f429 to 67d6171 Oct 1, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Oct 2, 2019
Copy link
Member

Trott left a comment

If a Collaborator wants to block this, I suggest making your opposition explicit with a "Request changes" review or a clear comment. As I said above, I'm generally inclined toward "leave it in experimental a bit longer" for anything where there's doubt, but there are certainly many people far more informed on this specific issue than I am....

@Trott Trott added the author ready label Oct 2, 2019
@guybedford guybedford referenced this pull request Oct 7, 2019
14 of 14 tasks complete
Copy link
Member

MylesBorins left a comment

LGTM

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 9, 2019

Landed in c0437d2

@Trott Trott closed this Oct 9, 2019
Trott added a commit that referenced this pull request Oct 9, 2019
This reverts commit ec8776d.

PR-URL: #29754
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 9, 2019

@nodejs/tsc Because this restores a flag that was removed/made unnecessay in Node.js 12.4.0, is this semver-major and should not land except as part of 13.0.0?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 9, 2019

@nodejs/tsc Because this restores a flag that was removed/made unnecessay in Node.js 12.4.0, is this semver-major and should not land except as part of 13.0.0?

Oh, wait, never mind, all of esm is experimental status so semver rules don't apply.

BridgeAR added a commit that referenced this pull request Oct 9, 2019
This reverts commit ec8776d.

PR-URL: #29754
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: TBD
@BridgeAR BridgeAR referenced this pull request Oct 10, 2019
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.