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: add warnings for experimental flags #30617

Closed
wants to merge 1 commit into from
Closed

module: add warnings for experimental flags #30617

wants to merge 1 commit into from

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented Nov 24, 2019

Fixes: #30600

Progress

--es-module-specifier-resolution=node has been added in the PR at #30678.

Warning Message

  • --experimental-json-modules
  • --experimental-wasm-modules
  • --experimental-resolve-self
  • --experimental-conditional-exports

Test Case

  • --experimental-json-modules
  • --experimental-wasm-modules
  • --experimental-resolve-self
  • --experimental-conditional-exports
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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 Nov 24, 2019
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Amazing work tackling this one... really appreciated.

Note that all the warnings need to have a boolean check that they have already been shown to ensure they only get shown once.

lib/internal/modules/esm/default_resolve.js Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
@pd4d10
Copy link
Contributor Author

pd4d10 commented Nov 25, 2019

@guybedford Thanks for your review!

I've noticed that there is already a function in lib/internal/util.js to ensure that experimental message only show once, by saving shown flags to a set and check it at next time.

function emitExperimentalWarning(feature) {

Shall we reuse this function?

@guybedford
Copy link
Contributor

guybedford commented Nov 25, 2019 via email

@MylesBorins MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 27, 2019
@addaleax
Copy link
Member

This LGTM code-wise 👍

src/module_wrap.cc Outdated Show resolved Hide resolved
@pd4d10
Copy link
Contributor Author

pd4d10 commented Nov 29, 2019

Hi @guybedford , I've updated the current progress at #30617 (comment).

There are some questions I haven't figured out and I'd like to ask you:

  • --es-module-specifier-resolution=node doesn't have experimental prefix. Shall we use the same warning message as other experimental flags? The current message is:
{{Feature name}} is an experimental feature. This feature could change at any time
  • --experimental-resolve-self and --experimental-conditional-exports was once tested in test/es-module/test-esm-exports.mjs, and removed in this PR: esm: Unflag --experimental-modules #29866. Shall we continue to write test cases for these two flags in this file(test-esm-exports.mjs), or create new one?

Thanks!

@guybedford
Copy link
Contributor

--es-module-specifier-resolution=node doesn't have experimental prefix. Shall we use the same warning message as other experimental flags? The current message is:

This has been added in the PR at #30678. We're discussing it at https://github.com/nodejs/node/pull/30678/files#r351640229.

Shall we continue to write test cases for these two flags in this file(test-esm-exports.mjs), or create new one?

The test-esm-exports.mjs test runs with the --experimental-modules flag which is currently an alias for the --experimental-resolve-self and --experimental-conditional-exports flags. So the test still includes them.

A new test for the warning messages only could be worthwhile though.

@pd4d10 pd4d10 changed the title WIP: module: add warnings for experimental flags module: add warnings for experimental flags Dec 1, 2019
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Mostly just wording fixes etc. Looking good.

src/node_process_events.cc Show resolved Hide resolved
src/node_process_events.cc Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
test/es-module/test-esm-wasm.mjs Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor

I also believe that the JS warning methods don't automatically dedupe themselves like this C++ one does. That seems to be an argument for having boolean flags at the calling points... but I'm not set either way on this.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This seems great to merge at this point to me. @pd4d10 just let me know when you are ready for the CI run.

test/common/fixtures.mjs Outdated Show resolved Hide resolved
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

It seems all the logic is there now, final comments are just on the wording of the messages.

test/es-module/test-esm-json.mjs Outdated Show resolved Hide resolved
src/node_process_events.cc Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 4, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joaocgreis
Copy link
Member

guybedford pushed a commit that referenced this pull request Dec 5, 2019
PR-URL: #30617
Fixes: #30600
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@guybedford
Copy link
Contributor

Landed in aa4c57a.

@guybedford guybedford closed this Dec 5, 2019
@pd4d10 pd4d10 deleted the patch-3 branch December 5, 2019 03:38
targos pushed a commit that referenced this pull request Dec 9, 2019
PR-URL: #30617
Fixes: #30600
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
PR-URL: #30617
Fixes: #30600
Reviewed-By: Guy Bedford <guybedford@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #30617
Fixes: #30600
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
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.

--experimental modules flags should have warnings
7 participants