module: add warnings for experimental flags

[pd4d10]

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

[guybedford]

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.

[pd4d10]

@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

functionemitExperimentalWarning(feature){

Shall we reuse this function?

[guybedford]

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

[addaleax]

This LGTM code-wise 👍

[pd4d10]

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]

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

[guybedford]

Mostly just wording fixes etc. Looking good.

[guybedford]

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.

[guybedford]

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

[guybedford]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[joaocgreis]

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

[guybedford]

Landed in aa4c57a.