process: add emitExperimentalWarning() #9042

evanlucas · 2016-10-11T23:22:40Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

process, internal

Description of change

This adds process.emitExperimentalWarning() which can used to
communicate to our users that a feature they are using is experimental
and can be changed/removed at any time. There is also a commit included
to use this api whenever the inspector is used.

Ref: #9036

jasnell

Good start but can be better. Left a few comments.

jasnell · 2016-10-11T23:41:03Z

lib/internal/process/warning.js

+class ExperimentalWarning extends Error {
+  constructor(feature) {
+    super(`${feature} is an experimental feature.`);
+    this.name = 'Warning';


Would recommend a name like ExperimentalWarning

jasnell · 2016-10-11T23:41:42Z

lib/internal/process/warning.js

@@ -4,6 +4,17 @@ const prefix = `(${process.release.name}:${process.pid}) `;

 exports.setup = setupProcessWarnings;

+class ExperimentalWarning extends Error {
+  constructor(feature) {
+    super(`${feature} is an experimental feature.`);


Perhaps a notice such as:

super(`${feature} is an experimental feature. Implementation details may change.`);

jasnell · 2016-10-11T23:43:19Z

lib/internal/process/warning.js

@@ -46,4 +57,8 @@ function setupProcessWarnings() {
    }
    process.nextTick(() => process.emit('warning', warning));
  };
+
+  process.emitExperimentalWarning = function(name) {
+    process.emitWarning(new ExperimentalWarning(name));


There's no reason to create a separate class for this, something like the following would work easily...

const msg = `${feature} is an experimental feature. ` + 'Implementation details may change.'; process.emitWarning(msg, 'ExperimentalWarning');

cjihrig

LGTM, but we're going to be stuck supporting process._useInspector for a long time.

evanlucas · 2016-10-12T14:59:13Z

@cjihrig yea, i'll fix that. I don't want to pollute process anymore than we have to

jasnell · 2016-10-12T15:20:16Z

test/parallel/test-process-emit-experimentalwarning.js

+const assert = require('assert');
+const exec = require('child_process').exec;
+
+if (process.argv[2] === 'child') {


Why use a child_process here?

The following should work fine...

process.on('warning', common.mustCall((warning) => { assert.strictEqual(warning.name, 'ExperimentalWarning'); assert.strictEqual(warning.message, message); })); process.emitExperimentalWarning('new_feature');

There are other existing tests that validate that warnings are printed to stderr appropriately.

Even better:

common.expectWarning('ExperimentalWarning', message); process.emitExperimentalWarning('new_feature'); process.emitExperimentalWarning('new_feature');

(call it twice to make sure it is not emitted more than once)

hm, even though warnings can be emitted more than once? I guess it could make sense to only emit the experimental warning for each feature once?

Should only be once I think.

Fishrock123

The test exemplifies my first comment imo, the fact that a child process is necessary.

Fishrock123 · 2016-10-13T14:19:56Z

lib/internal/bootstrap_node.js

+    // TODO(evanlucas) Remove this when v8_inspector is no longer experimental.
+    if (process._useInspector) {
+      process.emitExperimentalWarning('v8_inspector');
+    }


This should be after setupRawDebug() at minimum.

However, I think this probably needs to happen at the end of the first tick?

i.e. do we want user handlers to catch this?

ah yea, good point

So, process.emitWarning currently emits the warning using process.nextTick(). Will that be sufficient or does it need an additional setImmediate()?

Fishrock123 · 2016-10-13T14:21:41Z

lib/internal/process/warning.js

+
+  process.emitExperimentalWarning = function(feature) {
+    const msg = `${feature} is an experimental feature. ` +
+                'Implementation details may change.';


I think the wording should be improved... the exposed feature could change at any time.

This adds process.emitExperimentalWarning() which can used to communicate to our users that a feature they are using is experimental and can be changed/removed at any time. Ref: nodejs#9036

jasnell · 2016-10-19T16:00:39Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/4580/
LGTM if CI is green.

jasnell · 2016-10-26T19:56:17Z

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

jasnell · 2016-10-28T16:35:28Z

@evanlucas ... CI looks good. Want to get this landed?

Fishrock123 · 2016-10-28T19:26:21Z

I don't think this ever addressed my comment?

Fishrock123 · 2016-10-28T19:26:39Z

lib/internal/bootstrap_node.js

@@ -50,6 +50,11 @@

    _process.setupRawDebug();

+    // TODO(evanlucas) Remove this when v8_inspector is no longer experimental.
+    if (process.execArgv.indexOf('--inspect') !== -1) {
+      process.emitExperimentalWarning('v8_inspector');


gets emitted before a listener can be attached

even though the warning is emitted in a process.nextTick() (https://github.com/nodejs/node/blob/master/lib/internal/process/warning.js#L47)?

Yes, in both the REPL and case like node foo.js, the warning is emitted before user code is invoked. Setting a pre-load module using -r allows the warning event to be listened for.

ahhh ok, sorry about that. Will get that fixed this weekend. Thanks!

Fishrock123 · 2016-10-28T19:27:33Z

test/parallel/test-process-emit-experimentalwarning.js

+                'change at any time.';
+common.expectWarning('ExperimentalWarning', warning);
+process.emitExperimentalWarning('new_feature');
+process.emitExperimentalWarning('new_feature');


why twice here?

to make sure it is only emitted once

Fishrock123

the emit timing on that one needs to be addressed

rvagg · 2016-10-29T23:19:02Z

This is a good candidate for backporting to v4 and v6, what think you @nodejs/LTS?

jasnell · 2016-10-29T23:20:53Z

With exception to the DeprecationWarning pieces, yes I agree. process
warnings would be fairly easy to backport.

On Saturday, October 29, 2016, Rod Vagg notifications@github.com wrote:

This is a good candidate for backporting to v4 and v6, what think you
@nodejs/lts https://github.com/orgs/nodejs/teams/lts?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#9042 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eYq50vBAGWiWlD241FCJwhWlhBbMks5q49RtgaJpZM4KUNXY
.

rvagg · 2016-10-29T23:58:22Z

Oh ya, I forgot for a moment that the whole api was missing in v4. Easier case to limit backporting this bit to v6 at this stage probably.

This is currently an experimental feature, so we should make sure users are aware that it can be changed at any time. Ref: nodejs#9036

jasnell · 2017-03-01T02:22:15Z

@evanlucas ... still want to pursue this?

evanlucas · 2017-03-17T21:40:47Z

Going to close for now. I may have time in the near future to do this or not, but someone else can pick up if they want :]

mscdex · 2017-03-17T23:03:07Z

Also, perhaps we could use a Symbol to make it less accessible since it seems like this is for internal use only?

jasnell · 2017-03-20T17:24:50Z

I'll throw this on my todo list to look into

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 11, 2016

evanlucas added the process Issues and PRs related to the process subsystem. label Oct 11, 2016

jasnell requested changes Oct 11, 2016

View reviewed changes

evanlucas force-pushed the experimental branch from 4c7d7d1 to 2806e9e Compare October 12, 2016 01:46

cjihrig approved these changes Oct 12, 2016

View reviewed changes

jasnell reviewed Oct 12, 2016

View reviewed changes

Fishrock123 suggested changes Oct 13, 2016

View reviewed changes

process: add emitExperimentalWarning()

bb243f0

This adds process.emitExperimentalWarning() which can used to communicate to our users that a feature they are using is experimental and can be changed/removed at any time. Ref: nodejs#9036

evanlucas force-pushed the experimental branch from 2806e9e to a7365f9 Compare October 17, 2016 22:22

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell approved these changes Oct 28, 2016

View reviewed changes

Fishrock123 reviewed Oct 28, 2016

View reviewed changes

Fishrock123 suggested changes Oct 28, 2016

View reviewed changes

internal: emit experimental warning when using --inspect

0322d54

This is currently an experimental feature, so we should make sure users are aware that it can be changed at any time. Ref: nodejs#9036

evanlucas force-pushed the experimental branch from a7365f9 to 0322d54 Compare October 31, 2016 10:19

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 1, 2017

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

evanlucas closed this Mar 17, 2017

ssbrewster mentioned this pull request Sep 12, 2017

consider printing Experimental warnings for all Experimental APIs #9036

Closed

aliscco mentioned this pull request Nov 29, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#705

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#246

Open

This was referenced Nov 30, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#826

Open

[Snyk] Fix for 3 vulnerabilities aliscco/alisco-node#846

Open

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#903

Open

leonardoadame mentioned this pull request Dec 3, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#508

Open

process: add emitExperimentalWarning() #9042

process: add emitExperimentalWarning() #9042

Conversation

evanlucas commented Oct 11, 2016

Checklist

Affected core subsystem(s)

Description of change

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

evanlucas commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 19, 2016

jasnell commented Oct 26, 2016

jasnell commented Oct 28, 2016

Fishrock123 commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

rvagg commented Oct 29, 2016

jasnell commented Oct 29, 2016

rvagg commented Oct 29, 2016

jasnell commented Mar 1, 2017

evanlucas commented Mar 17, 2017

mscdex commented Mar 17, 2017

jasnell commented Mar 20, 2017