module: add warnings for experimental flags by pd4d10 · Pull Request #30617 · nodejs/node · GitHub

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

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.

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

node/lib/internal/util.js

Line 168 in 0646eda

function emitExperimentalWarning(feature) {

Shall we reuse this function?

Yes that is the function to use. Is there a version of that for C++ to use or do we have to create our own?

…

On Mon, Nov 25, 2019 at 09:01 Rongjian Zhang ***@***.***> wrote: @guybedford <https://github.com/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. https://github.com/nodejs/node/blob/0646eda4fc0affb98e13c30acb522e63b7fd6dde/lib/internal/util.js#L168 Shall we reuse this function? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30617?email_source=notifications&email_token=AAESFSRMRNLUXRSRIWPZO5LQVPLE3A5CNFSM4JQ5LIBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFCPVPQ#issuecomment-558168766>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSSSUBHPZY4QTV34IJDQVPLE3ANCNFSM4JQ5LIBA> .

This LGTM code-wise 👍

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!

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

Mostly just wording fixes etc. Looking good.

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.

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

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

Fixes: #30600

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

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

Windows CI re-run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/31996/

PR-URL: #30617 Fixes: #30600 Reviewed-By: Guy Bedford <guybedford@gmail.com>

Landed in aa4c57a.

PR-URL: #30617 Fixes: #30600 Reviewed-By: Guy Bedford <guybedford@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 24, 2019

guybedford reviewed Nov 25, 2019

View reviewed changes

Comment thread lib/internal/modules/esm/default_resolve.js Outdated

Comment thread src/module_wrap.cc Outdated

Comment thread src/module_wrap.cc Outdated

MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 27, 2019

guybedford reviewed Nov 27, 2019

View reviewed changes

Comment thread src/module_wrap.cc Outdated

guybedford mentioned this pull request Nov 29, 2019

esm: make specifier flag clearly experimental #30678

Closed

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

pd4d10 requested a review from guybedford December 1, 2019 13:41

pd4d10 commented Dec 1, 2019

View reviewed changes

Comment thread test/common/fixtures.mjs Outdated

guybedford reviewed Dec 2, 2019

View reviewed changes

Comment thread src/node_process_events.cc Outdated

Comment thread src/node_process_events.cc Outdated

Comment thread src/module_wrap.cc Outdated

Comment thread test/es-module/test-esm-wasm.mjs Outdated

guybedford approved these changes Dec 3, 2019

View reviewed changes

guybedford reviewed Dec 3, 2019

View reviewed changes

Comment thread test/common/fixtures.mjs Outdated

guybedford requested changes Dec 3, 2019

View reviewed changes

Comment thread test/es-module/test-esm-json.mjs Outdated

Comment thread src/node_process_events.cc Outdated

Comment thread src/module_wrap.cc Outdated

Comment thread lib/internal/modules/cjs/loader.js Outdated

module: add warnings for experimental flags

59b1a3f

Fixes: #30600

guybedford approved these changes Dec 4, 2019

View reviewed changes

guybedford added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 4, 2019

guybedford pushed a commit that referenced this pull request Dec 5, 2019

module: add warnings for experimental flags

aa4c57a

PR-URL: #30617 Fixes: #30600 Reviewed-By: Guy Bedford <guybedford@gmail.com>

guybedford closed this Dec 5, 2019

pd4d10 deleted the patch-3 branch December 5, 2019 03:38

pd4d10 mentioned this pull request Dec 5, 2019

make all experimental warning messages consistent #30803

Closed

targos pushed a commit that referenced this pull request Dec 9, 2019

module: add warnings for experimental flags

851f313

PR-URL: #30617 Fixes: #30600 Reviewed-By: Guy Bedford <guybedford@gmail.com>

MylesBorins mentioned this pull request Dec 13, 2019

v13.4.0 proposal #30937

Merged

MylesBorins pushed a commit that referenced this pull request Jan 12, 2020

module: add warnings for experimental flags

f0850a3

PR-URL: #30617 Fixes: #30600 Reviewed-By: Guy Bedford <guybedford@gmail.com>

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

BethGriggs pushed a commit that referenced this pull request Feb 6, 2020

module: add warnings for experimental flags

dc084f9

PR-URL: #30617 Fixes: #30600 Reviewed-By: Guy Bedford <guybedford@gmail.com>

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

module: add warnings for experimental flags by pd4d10 · Pull Request #30617 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: add warnings for experimental flags - #30617

module: add warnings for experimental flags#30617pd4d10 wants to merge 1 commit intomasternodejs/node:masterfrom unknown repositoryCopy head branch name to clipboard

Conversation

pd4d10 commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Warning Message

Test Case

Checklist

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pd4d10 commented Nov 25, 2019

Uh oh!

guybedford commented Nov 25, 2019 via email

Uh oh!

addaleax commented Nov 27, 2019

Uh oh!

Uh oh!

pd4d10 commented Nov 29, 2019

Uh oh!

guybedford commented Nov 29, 2019

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybedford commented Dec 2, 2019

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 4, 2019

Uh oh!

nodejs-github-bot commented Dec 5, 2019

Uh oh!

joaocgreis commented Dec 5, 2019

Uh oh!

guybedford commented Dec 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

module: add warnings for experimental flags#30617
pd4d10 wants to merge 1 commit into
masterfrom
unknown repository

pd4d10 commented Nov 24, 2019 •
edited
Loading