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 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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 28, 2019
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@addaleax addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 28, 2019
@littledan
Copy link

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
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
Copy link

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
Copy link
Collaborator

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

@Trott Trott Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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 Trott added the wip Issues and PRs that are still a work in progress. label Oct 1, 2019
@Trott
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 json-modules-flag branch 4 times, most recently from 39e2f6b to 683f429 Compare October 1, 2019 04:50
@guybedford guybedford removed the wip Issues and PRs that are still a work in progress. label Oct 1, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 2, 2019
@guybedford guybedford mentioned this pull request Oct 7, 2019
14 tasks
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott
Copy link
Member

Trott commented Oct 9, 2019

Landed in c0437d2

@Trott Trott closed this Oct 9, 2019
Trott pushed 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
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
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 pushed 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 BridgeAR mentioned 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 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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants