fs: Revert throw on invalid callbacks #12976
Conversation
nodejs-github-bot
added
the
fs
|
/cc @feross would you consider adding a lint rule to standard to help with this? |
|
While I generally prefer throwing, I'm sensitive to the ecosystem impact enough to give a very reluctant +1 to this. |
|
Thank you @MylesBorins |
|
🙏 thank you @MylesBorins! |
|
@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. |
|
@MylesBorins out of interest, is there a reason you didn't just use |
|
UGH... I accidentally made the branch on upstream. Sorry y'all. Should we close / reopen? |
|
@MylesBorins fair enough. I'll raise a separate issue to discuss, don't want to clutter this PR (EDIT: #12979)
No need to reopen IMO. |
|
Was this deprecated in Node 6? If so, people have had fair warning? |
|
@Fishrock123 We deprecated this in Node 7, with #7897. |
@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:
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:
|
|
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. |
|
-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. |
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. |
|
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>
.
|
|
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. |
|
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. |
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 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. |
|
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 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. |
|
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:
|
Oh, yay, I actually argued against it's usage specifically to avoid this confusion we're having now. :-D 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.) |
Just want to point out that |
|
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? |
This reverts 4cb5f3d 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 PR-URL: #12976 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
This reverts 4cb5f3d 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 PR-URL: #12976 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
|
Thanks @MylesBorins! |
This reverts commit 8250bfd. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
The callback should run in the global scope and not in the FSReqWrap context. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
This reverts commit 8250bfd. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
The callback should run in the global scope and not in the FSReqWrap context. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
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