fs: Revert throw on invalid callbacks

[MylesBorins]

Based on community feedback I think we should consider reverting this
change. We should explore how this could be solved via linting rules.

Refs: #12562

/cc @nodejs/ctc

[MylesBorins]

/cc @feross would you consider adding a lint rule to standard to help with this?

[jasnell]

While I generally prefer throwing, I'm sensitive to the ecosystem impact enough to give a very reluctant +1 to this.

[toddself]

Thank you @MylesBorins

[lrlna]

🙏 thank you @MylesBorins!

[MylesBorins]

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

[jasnell]

@MylesBorins ... given how popular this proposed change has been in the @nodejs/ctc we may need to give this a bit more time for review before it lands. I'm putting this on the 8.0.0 milestone tho.

[gibfahn]

@MylesBorins out of interest, is there a reason you didn't just use git revert 4cb5f3daa350421e4eb4622dc818633d3a0659b3 and thus go with the standard commit message:

Revert "fs: throw on invalid callbacks for async functions" This reverts commit 4cb5f3daa350421e4eb4622dc818633d3a0659b3. 

[MylesBorins]

@gibfahn I looked at prior revert comments and they were inconsistent. The default revert message was longer than max title length

3441a41

[MylesBorins]

UGH... I accidentally made the branch on upstream. Sorry y'all. Should we close / reopen?

[gibfahn]

@MylesBorins fair enough. I'll raise a separate issue to discuss, don't want to clutter this PR (EDIT:#12979)

UGH... I accidentally made the branch on upstream. Sorry y'all. Should we close / reopen?

No need to reopen IMO.

[Fishrock123]

Was this deprecated in Node 6? If so, people have had fair warning?

[thefourtheye]

@Fishrock123 We deprecated this in Node 7, with #7897.

[retrohacker]

people have had fair warning?

@Fishrock123 in some circumstances I would agree: when the changes are desperately needed and can only sufficiently be addressed by core. In this case, there are other paths forward that don't include breaking behaviour that userland relies on. Though it wasn't explicitly documented, optional callbacks are common enough that users did rely on that behaviour.

I agree that Node.js should help users write better and safer code as @jasnell pointed out, though I would add the caveat this should only be done if it can be a minor/patch release, saving breaking changes for exceptional circumstances. In recent history, I know of two exceptional conditions where I personally would reluctantly agree (as a passive onlooker) with a decision to break userland:

1. Compatibility (not necessarily support) for ES6
2. Fixing APIs where there are known proof-of-concepts for exploits in the wild that affect a significant portion of userland

To my knowledge, this isn't (2).

In both of the cases above, giving an ample window for feedback and shipping irrespective of cost is valid as the reward to userland is greater than the cost. I'm not convinced this is the case here.

In a much lighter spirit:

But, Mr Dent, the plans have been available in the local planning office for the last nine months.
-- Douglas Adams

[toddself]

I would like to say that I very specifically don't use odd-numbered releases since they'll never be released into LTS. I feel like deprecating things with a warning in an odd number to remove in an even number is going to cause a lot of issues like this where this feels "very sudden" to me.

[cjihrig]

Rubber stamp LGTM, assuming it's just a normal revert and CI is happy.

[not-an-aardvark]

-0. I think we've given users ample warning here by showing a deprecation message for an entire release, and there is a trivial workaround (adding an empty callback) that works in all Node versions. Forgetting to pass a callback is likely to be a source of errors for people who aren't familiar with Node or callbacks.

How large is the ecosystem breakage with this change? I think there is a point where it's unreasonable for Node to keep its API error-prone for all new users, just so that we can accommodate a few modules that continue to rely on undocumented behavior rather than making a one-line change.

[sindresorhus]

How large is the ecosystem breakage with this change? I think there is a point where it's unreasonable for Node to keep its API error-prone for all new users, just so that we can accommodate a few modules that continue to rely on undocumented behavior rather than making a one-line change.

Before this PR is merged, I think there should be proof that it actually breaks a large amount of packages. Currently it's just based on the opinion of a few outspoken users.

[gibfahn]

Before this PR is merged, I think there should be proof that it actually breaks a large amount of packages. Currently it's just based on the opinion of a few outspoken users.

@ChALkeR is this something that that Gzemnid can give info on?

[MylesBorins]

The biggest challenge here is going to be finding out not only how many packages will break from this, but how many packages depended on a module that will break from this. As we have seen in the past with graceful-fs the fallout can be suprisingly large. I'm all for doing some research on this, but I do want to stress we have a limited amount of time to do so. It does feel like we should err on the side of proving it won't break things prior to landing changes like this in the future

…

On May 12, 2017 11:19 AM, "Gibson Fahnestock" ***@***.***> wrote: Before this PR is merged, I think there should be proof that it actually breaks a large amount of packages. Currently it's just based on the opinion of a few outspoken users. @ChALkeR <https://github.com/chalker> is this something that that Gzemnid <https://github.com/ChALkeR/Gzemnid> can give info on? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12976 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV4yXRvEwDk3bpHDgzOBIExbkchDsks5r5CQZgaJpZM4NYNSI> .

[targos]

I agree with @MylesBorins. We should go ahead with the revert and then do the research to decide if we can introduce this change in Node 9.

[jasnell]

As the person doing the work to cut the 8.0.0 release, I agree with @MylesBorins. I fully support throwing when a callback is not provided but much prefer that we err on the side of caution in this case.

That said, if we do decide to go ahead with reverting this now and landing it again in master for 9.x, I do not expect that the small handful of module authors whose modules are most affected by this will be at all interested in updating their code. This is based on the kinds of comments that I have seen being made by those authors on Twitter about this and similar changes. This means that whether we make this change now or whether we make this change later, the same users of those modules will continue to be affected negatively. The only mitigation step we can take for that is to give those users more time to move to an alternative that would not be broken by this change.

[toddself]

Omitting the callback kills the program when the operation fails

Being able to write code that looks correct but actually swallows errors is an undesirable feature for an API.

This does not swallow errors. You are very much warned when this fails by the thrown exception. The use-case we're talking about here are when you don't care about success.

There are likely hundreds of tools out there where the last operation is writing something to disk -- adding a callback to deal with success here is pointless -- success is noted by the successful exiting of the software. Errors are STILL presented to end users since the program will throw them.

I'm not sympathetic to the "I don't want to change anything" dinosaur crowd, though.

I am extremely happy to deal with change when change is warranted -- evolving node and JavaScript he Language™ is a great idea! Changing stable modules that causes breakages for a matter of stylistic preference is not being a dinosaur in the slightest imho.

[Trott]

Doesn't our deprecation policy require two major releases with runtime warnings before we remove something? https://github.com/nodejs/node/blob/98609fc1c4635f02f8f6ef9dd079207a1204b9a1/COLLABORATOR_GUIDE.md#deprecations (It's a bit confusing to me, because End-of-Life Deprecation seems to be the same as Runtime Deprecation, just that it has to happen after Runtime Deprecation. So it seems to be a name change. If I'm not totally misunderstanding our policy, it might help to simplify it by removing End-of-Life Deprecation and saying "Runtime Deprecations have to be in place for two major releases before a working API is removed". And even if I am misreading it, that may be a sign it needs to be reworked for clarity a bit?

ANYWAY, assuming I'm not terribly wrong (which is obviously a possibility): Since the runtime deprecation landed in Node.js 7, that would seem to suggest that removal of the ability to use these APIs without a callback shouldn't happen any earlier than Node.js 9, or at least not without CTC approval or something.

[not-an-aardvark]

My interpretation was that "end-of-life deprecation" basically just means "this was deprecated before but now it's removed". So this feature was runtime deprecated in 7.x, and now we're deciding whether to end-of-life deprecate it (i.e. remove it). However, I'm not sure I have the definitions right.

[Trott]

My interpretation was that "end-of-life deprecation" basically just means "this was deprecated before but now it's removed". So this feature was runtime deprecated in 7.x, and now we're deciding whether to end-of-life deprecate it (i.e. remove it). However, I'm not sure I have the definitions right.

Yeah, that would seem to be a straightforward inference from the name and was what I initially thought. I think it's just poorly named, though. (And now I just hope it doesn't turn out that I'm the one that came up with the name. :-D)

The main reasons for my interpretation (that "end-of-life deprecation" is basically "leave the runtime deprecation there for a second major release") are:

• The text defines "end-of-life deprecation" as something that is ready to be removed rather than something that has been removed.

• It defines "deprecation" as the identification of APIs that "that may be removed or modified in non-backwards compatible ways in a future major release of Node.js". So if it's a "deprecation", it hasn't yet been modified or removed.

• I seem to recall @jasnell specifically making the case that stuff needed to emit runtime deprecations for two major releases. He's the one that wrote the policy mostly, so I would think that feature made it in. I hope I'm not just imagining all that though.

[Trott]

I think it's just poorly named, though. (And now I just hope it doesn't turn out that I'm the one that came up with the name. :-D)

Oh, yay, I actually argued against it's usage specifically to avoid this confusion we're having now. :-D

#7964 (comment)

I think the conversation in that pull request (where the deprecation policy was written) makes it clear that deprecation warnings (emitted at runtime) need to stay in place for two major releases before an API is removed or modified in a way that isn't backwards compatible. I think it should probably also make explicit that the CTC can make exceptions. (The removal of the legacy debug API recently would be a good example of when/why that can be necessary.)

[Trott]

LGTM if CI is green.

[thefourtheye]

As far as ecosystem breakage: this breaks watchify, which is currently depended upon by 544 packages according to npm right now and ~700k downloads a month. Watchify is the browserify ecosystem's solution for bundle rebuilding in development.

There are 106 copies of watchify currently installed on my machine under ~/src, many of them are as a dependent of a dependency -- and none of those can update until a new version of watchify gets created.

Just want to point out that watchify module is not directly impacted because of this change. One of test files offend this change. That will not impact the module users.

[addaleax]

Ping @MylesBorins – do you want to go ahead and land this? There is one collaborator who has 👎’ed the PR, but I think that’s it as far as objections go.

[gibfahn]

Ping @MylesBorins – do you want to go ahead and land this? There is one collaborator who has 👎’ed the PR, but I think that’s it as far as objections go.

@vkurchatkin are you -1 on this landing?

[MylesBorins]

I've landed this in 8250bfd

I've also cherry-picked to v8.x-staging in eebae66@jasnell it might make sense to drop this and the original commit from the 8.x line altogether, not sure what you should prefer

[jasnell]

Thanks @MylesBorins!