fs: warn instead of throwing if no callback #7897

thefourtheye · Jul 27, 2016

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

9359de9
made sure that if no callback function is passed to asynchronous
functions, the system will throw.

But, this breaks a lot of existing modules including npm (Refer:
#7846). Till all the necessary
dependencies use the async functions properly, no module can work.

So, this patch issues a deprecation warning, instead of throwing and
the throwing behaviour will be restored in the next major release.

cc @ChALkeR @nodejs/collaborators @nodejs/ctc

thefourtheye · Jul 27, 2016

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

MylesBorins · Jul 27, 2016

MylesBorins reviewed Jul 27, 2016

View changes

MylesBorins · Jul 27, 2016

Sign in to view

-  if (typeof cb !== 'function') {
+  if (typeof cb === 'undefined') {
+    // TODO throw error instead of deprecation warning in v8.0.0
+    let m = 'async function calls without valid callbacks is deprecated.';


nit: are deprecated

MylesBorins · Jul 27, 2016

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker-xunit/39/

I like the approach here. +1 for this over the revert

yorkie · Jul 27, 2016

yorkie reviewed Jul 27, 2016

View changes

yorkie · Jul 27, 2016

Sign in to view

@@ -95,7 +98,14 @@ function sanitizeFD(fd) {
 // for callbacks that are passed to the binding layer, callbacks that are
 // invoked from JS already run in the proper scope.
 function makeCallback(cb) {
-  if (typeof cb !== 'function') {
+  if (typeof cb === 'undefined') {
+    // TODO throw error instead of deprecation warning in v8.0.0


I did remember the other TODO/FIXME/etc.'s format is not like this, suggest:

// TODO(thefourtheye): throw error instead of deprecation warning in v8.0.0

yorkie · Jul 27, 2016

yorkie reviewed Jul 27, 2016

View changes

yorkie · Jul 27, 2016

Sign in to view

@@ -40,6 +41,8 @@ const isWindows = process.platform === 'win32';

 const errnoException = util._errnoException;

+const NOOP = function() {};


noop in lower case would be better that keeps consistent with https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=NOOP.

claudiorodriguez · Jul 28, 2016

citgm failing:

+ npm set progress=false
...
npm ERR! npm  v3.10.3
...
npm ERR! cb is not a function

thefourtheye · Jul 28, 2016

Do you guys know how I can locally reproduce this problem?

targos · Jul 28, 2016

Do you guys know how I can locally reproduce this problem?

make test-npm

thefourtheye · Jul 28, 2016

~~I think when npm/npm#13497 lands and gets integrated to Node.js Core, this problem will be solved.~~

Wait, this should not fail though. I'll investigate further, a little later.

cjihrig · Aug 1, 2016

cjihrig reviewed Aug 1, 2016

View changes

cjihrig · Aug 1, 2016

Sign in to view

@@ -95,7 +98,15 @@ function sanitizeFD(fd) {
 // for callbacks that are passed to the binding layer, callbacks that are
 // invoked from JS already run in the proper scope.
 function makeCallback(cb) {
-  if (typeof cb !== 'function') {
+  if (typeof cb === 'undefined') {


You can just compare cb === undefined.

RReverser · Aug 2, 2016

RReverser reviewed Aug 2, 2016

View changes

RReverser · Aug 2, 2016

Sign in to view

@@ -40,6 +41,8 @@ const isWindows = process.platform === 'win32';

 const errnoException = util._errnoException;

+const noop = function() {};
+
 var printDeprecation;
 try {
  printDeprecation = require('internal/util').printDeprecationMessage;


You can put var right here (to avoid duplication) or change top-level declaration to let.

jasnell · Aug 4, 2016

jasnell reviewed Aug 4, 2016

View changes

jasnell · Aug 4, 2016

Sign in to view

+    // Refer the discussion: https://github.com/nodejs/node/pull/7846
+    let m = 'async function calls without valid callbacks are deprecated.';
+    if (!process.traceDeprecation)
+      m += ' Use --trace-deprecation to find the function responsible.';


I'm not sure this additional part about --trace-deprecation is necessary but can live with it.

jasnell · Aug 4, 2016

jasnell reviewed Aug 4, 2016

View changes

jasnell · Aug 4, 2016

Sign in to view

+  if (typeof cb === 'undefined') {
+    // TODO(thefourtheye): throw error instead of deprecation warning in v8.0.0
+    // Refer the discussion: https://github.com/nodejs/node/pull/7846
+    let m = 'async function calls without valid callbacks are deprecated.';


s/async/Asynchronous

jasnell · Aug 4, 2016

A test case validating that the deprecation warning is emitted would be good. With that added this LGTM

addaleax · Aug 4, 2016

@thefourtheye As expected, this conflicts now… I think it’s okay to apply the changes that got reverted here again (with the intention to squash in the warnings?). Sorry for the trouble the process is causing you!

jasnell · Aug 9, 2016

ping @thefourtheye

thefourtheye · Aug 12, 2016

Updated with the deprecation warning alone. PTAL people.

silverwind · Aug 12, 2016

silverwind reviewed Aug 12, 2016

View changes

silverwind · Aug 12, 2016

Sign in to view

@@ -112,10 +113,13 @@ function maybeCallback(cb) {
 // invoked from JS already run in the proper scope.
 function makeCallback(cb) {
  if (cb === undefined) {
+    // TODO(thefourtheye) throw error instead of warning in major version > 7
+    let m = 'asynchronous function calls without callbacks is deprecated.';


are deprecated

or even better, make it singular.

@silverwind Done.

jasnell · Aug 18, 2016

~~LGTM with @silverwind's nit addressed~~

Actually, let me take that back... this uses internal which means it would end up being blocked by the graceful-fs issues. Instead of using printDeprecationNotice, this should be modified to use the new process.emitWarning() API.

For instance:

process.emitWarning(depMessage, 'DeprecationWarning');

By passing DeprecationWarning as the second argument, this will be handled as a normal deprecation.

ping @thefourtheye

thefourtheye · Aug 19, 2016

@jasnell I modified it to use printDeprecation function which already takes care of the graceful-fs problem. Is that okay?

jasnell · Aug 19, 2016

@thefourtheye ... That partially handles it. See #8166. I'm attempting to move things away from using the internal util printDeprecationMessage.

jasnell · Aug 26, 2016

jasnell reviewed Aug 26, 2016

View changes

jasnell · Aug 26, 2016

Sign in to view

+process.once('warning', common.mustCall((warning) => {
+  assert.strictEqual(
+    warning.message,
+    'calling an asynchronous function without callback is deprecated.'


nit: s/calling/Calling

jasnell · Aug 26, 2016

LGTM with a nit if CI is green.

thefourtheye · Aug 26, 2016

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

yorkie · Aug 26, 2016

LGTM if CI is green

jasnell · Aug 26, 2016

@nodejs/ctc ... as a semver-major this needs another CTC signoff...

addaleax · Aug 27, 2016

LGTM

thefourtheye · Aug 27, 2016

Thanks for the review people. Landed in f8f283b

thefourtheye added fs semver-major labels Jul 27, 2016

thefourtheye referenced this pull request Jul 28, 2016
Closed
Revert fs changes #7846
2 of 2 tasks complete

addaleax added this to the 7.0.0 milestone Aug 8, 2016

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch 2 times, most recently Aug 12, 2016

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch Aug 19, 2016

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch 2 times, most recently Aug 19, 2016

fs: warn if no callback is passed to async calls
This patch issues a deprecation warning, if an asynchronous function is called without a callback function.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 800C90E774692D78 Learn about signing commits

3e97e2c

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch to 3e97e2c Aug 26, 2016

thefourtheye closed this Aug 27, 2016

thefourtheye deleted the thefourtheye:fs-warn-instead-of-throwing branch Aug 27, 2016

jasnell referenced this pull request Oct 14, 2016
Closed
v7.0.0 Proposal #9099

goliney referenced this pull request Oct 25, 2016
Merged
Node v7 support #174

thefourtheye referenced this pull request May 11, 2017
Merged
fs: Revert throw on invalid callbacks #12976

gibfahn referenced this pull request Jun 15, 2017
Closed
Auditing for 6.11.1 #230
2 of 3 tasks complete

nodejs/node

Join GitHub today

fs: warn instead of throwing if no callback #7897

Conversation

thefourtheye commented Jul 27, 2016

Checklist

Affected core subsystem(s)

Description of change

thefourtheye added fs semver-major labels Jul 27, 2016

This comment has been minimized.

thefourtheye commented Jul 27, 2016

MylesBorins reviewed Jul 27, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MylesBorins commented Jul 27, 2016

yorkie reviewed Jul 27, 2016 View changes

This comment has been minimized.

This comment has been minimized.

yorkie reviewed Jul 27, 2016 View changes

This comment has been minimized.

This comment has been minimized.

thefourtheye referenced this pull request Jul 28, 2016

This comment has been minimized.

claudiorodriguez commented Jul 28, 2016

This comment has been minimized.

thefourtheye commented Jul 28, 2016

This comment has been minimized.

targos commented Jul 28, 2016

This comment has been minimized.

thefourtheye commented Jul 28, 2016 • edited

cjihrig reviewed Aug 1, 2016 View changes

This comment has been minimized.

RReverser reviewed Aug 2, 2016 View changes

This comment has been minimized.

jasnell reviewed Aug 4, 2016 View changes

This comment has been minimized.

jasnell reviewed Aug 4, 2016 View changes

This comment has been minimized.

This comment has been minimized.

jasnell commented Aug 4, 2016

This comment has been minimized.

addaleax commented Aug 4, 2016

addaleax added this to the 7.0.0 milestone Aug 8, 2016

This comment has been minimized.

jasnell commented Aug 9, 2016

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch 2 times, most recently Aug 12, 2016

This comment has been minimized.

thefourtheye commented Aug 12, 2016

silverwind reviewed Aug 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jasnell commented Aug 18, 2016 • edited

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch Aug 19, 2016

This comment has been minimized.

thefourtheye commented Aug 19, 2016

This comment has been minimized.

jasnell commented Aug 19, 2016

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch 2 times, most recently Aug 19, 2016

jasnell reviewed Aug 26, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jasnell commented Aug 26, 2016

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch to 3e97e2c Aug 26, 2016

This comment has been minimized.

thefourtheye commented Aug 26, 2016

This comment has been minimized.

yorkie commented Aug 26, 2016

This comment has been minimized.

jasnell commented Aug 26, 2016

This comment has been minimized.

addaleax commented Aug 27, 2016

This comment has been minimized.

thefourtheye commented Aug 27, 2016

thefourtheye closed this Aug 27, 2016

thefourtheye deleted the thefourtheye:fs-warn-instead-of-throwing branch Aug 27, 2016

thefourtheye added a commit that referenced this pull request Aug 27, 2016

MylesBorins reviewed Jul 27, 2016

View changes

yorkie reviewed Jul 27, 2016

View changes

yorkie reviewed Jul 27, 2016

View changes

thefourtheye commented Jul 28, 2016 •

edited

cjihrig reviewed Aug 1, 2016

View changes

RReverser reviewed Aug 2, 2016

View changes

jasnell reviewed Aug 4, 2016

View changes

jasnell reviewed Aug 4, 2016

View changes

silverwind reviewed Aug 12, 2016

View changes

jasnell commented Aug 18, 2016 •

edited

jasnell reviewed Aug 26, 2016

View changes

thefourtheye force-pushed the thefourtheye:fs-warn-instead-of-throwing branch to `3e97e2c` Aug 26, 2016