Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: add missed FSWatcher.prototype.start deprecation #29989

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 16, 2019

#29905 should have had a runtime deprecation.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@jasnell jasnell added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Oct 16, 2019
@jasnell jasnell mentioned this pull request Oct 16, 2019
4 tasks
Type: Runtime

The `FSWatcher.prototype.start()` function is an undocumented internal API that
does not make ense to use in userland.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
does not make ense to use in userland.
does not make sense to use in userland.


Type: Runtime

The `FSWatcher.prototype.start()` function is an undocumented internal API that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional suggestion:

Suggested change
The `FSWatcher.prototype.start()` function is an undocumented internal API that
`FSWatcher.prototype.start()` is an undocumented internal API that

@Trott
Copy link
Member

Trott commented Oct 16, 2019

@nodejs/tsc

@Trott

This comment has been minimized.

@Trott Trott added this to the 13.0.0 milestone Oct 16, 2019
Copy link
Contributor

@lholmquist lholmquist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the already suggested changes

@addaleax
Copy link
Member

I don’t think this is necessary, and if you are concerned about breakage, I would really prefer adding

FSWatcher.prototype.start = () => {};

and leaving it at that indefinitely rather than all the churn that comes with deprecations (and ditto for #29971).

@jasnell
Copy link
Member Author

jasnell commented Oct 16, 2019

I understand the objection @addaleax ... I think it may be good to change the deprecation policy to allow for exceptions to be made for apis that really should have been internal. For this particular change, perhaps a compromise and make it..

FSWatcher.prototype.start = deprecate(()->{}, 'msg....', 'DEP0XXX')

@addaleax
Copy link
Member

@jasnell My point isn’t quite as much that this should have been internal (and I wouldn’t be a fan of a general exception for that in our policies), but rather that a) it’s effectively a no-op from the userland perspective anyway, so the likelihood that it’s used in practice is close to zero, and b) that ultimately maintaining an explicit no-op or an alias indefinitely is generally always going to be cheaper than any kind of deprecation efforts.

Ultimately, you can feel free to pick what you think is best here and go with that.

@jasnell
Copy link
Member Author

jasnell commented Oct 16, 2019

What I'm thinking is landing the deprecation with a non-op function for the v14 line but moving it to end-of-line immediately in v15. That is, unless there is @nodejs/tsc consensus just to leave it as is without the deprecation. I'm good either way.

@jasnell jasnell added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 16, 2019
@jasnell
Copy link
Member Author

jasnell commented Oct 16, 2019

Flagging for tsc review. We don't necessarily need it on the agenda if we can get opinions here in thread

@lholmquist
Copy link
Contributor

I'm sort of torn on what should be done. I can agree with both sides. This function is really a no-op if called in userland, and an instance of this particular object can't be instantiated directly, only through the starting of a file watch. It is undocumented, so the only way someone would know about it, is if they looked in the code. So i can see the argument for just making in private since there might not be people using it.

On the other hand, adding in a deprecation would atleast make it not break for those users who are using it and not realizing it is a noop(or those that are and do know). But it is coming in a semver-major release, so breaking changes could be expected

I'm not a tsc member, but as the original author of the previous PR, i just wanted to add my thoughts.

@Trott
Copy link
Member

Trott commented Oct 18, 2019

I'm in favor of just removing this (and the other one). If that means the TSC has to formally approve an exception to the deprecation policy, I'd support that.

@Trott Trott removed this from the 13.0.0 milestone Oct 18, 2019
@Trott
Copy link
Member

Trott commented Oct 18, 2019

I'm in favor of just removing this (and the other one). If that means the TSC has to formally approve an exception to the deprecation policy, I'd support that.

IIUC, it seems like @addaleax and I are both on the "this doesn't need a formal deprecation, although we won't fight it either" side of the issue. @jasnell is "this should technically get a deprecation and I would slightly prefer that but maybe we can make an exception if there's TSC consensus on that".

Other than that, no other @nodejs/tsc members have weighed in yet. It would be good to get some more opinions, especially with the 13.0.0 release looming....

@targos
Copy link
Member

targos commented Oct 18, 2019

I think "this doesn't need a formal deprecation, although I won't fight it either"

@cjihrig
Copy link
Contributor

cjihrig commented Oct 18, 2019

I'm also on team "this doesn't need a formal deprecation, although I won't fight it either."

@ChALkeR
Copy link
Member

ChALkeR commented Oct 22, 2019

I would probably prefer a deprecation for at least one release, and not making this an exception -- because I don't see strong reasons for making this an exception.

But if you are counting votes -- I'm not going to push that if there is an informed consensus on just removing that, so that is closer to an "abstain".

@lholmquist
Copy link
Contributor

Since this is on the agenda for the tsc meeting later, i thought i would summarize the current vote count. it looks like it is:

4 - "this doesn't need a formal deprecation, although I won't fight it either."
1 - deprecation process
1 - abstain

@addaleax
Copy link
Member

I likely won’t be in the meeting later, but to re-iterate: While I’m kind of in the “I won’t fight it” camp, what I do want is for our processes to be changed for cases like this:

When maintaining an alias or making something a stub/no-op is a possibility, then we should just not runtime-deprecate at all, because the former is generally going to come with lower maintenance cost, even if the alias/stub sticks around for a long time.

@mhdawson
Copy link
Member

I'm in agreement with @addaleax's suggestion. It seems the lowest risk/effort.

@Trott
Copy link
Member

Trott commented Oct 28, 2019

Per TSC discussion and apparent consensus, I'm closing this. Feel free to re-open if there's anyone who feels strongly that otherwise.

Refs: nodejs/TSC#771 (comment)

@Trott Trott closed this Oct 28, 2019
@Trott Trott removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 28, 2019
@lholmquist
Copy link
Contributor

lholmquist commented Oct 28, 2019

Would it be good then to add the no-op stub, even though it is already part of the 13.x release? I'll create a new PR for that

Trott added a commit to Trott/io.js that referenced this pull request Oct 30, 2019
Avoid Runtime Deprecations when an alias or a stub/no-op will suffice.
An alias or stub will have lower maintenance costs for end users.

Refs: nodejs#29989 (comment)

PR-URL: nodejs#30153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 5, 2019
Avoid Runtime Deprecations when an alias or a stub/no-op will suffice.
An alias or stub will have lower maintenance costs for end users.

Refs: #29989 (comment)

PR-URL: #30153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 8, 2019
Avoid Runtime Deprecations when an alias or a stub/no-op will suffice.
An alias or stub will have lower maintenance costs for end users.

Refs: #29989 (comment)

PR-URL: #30153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
Avoid Runtime Deprecations when an alias or a stub/no-op will suffice.
An alias or stub will have lower maintenance costs for end users.

Refs: #29989 (comment)

PR-URL: #30153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
Avoid Runtime Deprecations when an alias or a stub/no-op will suffice.
An alias or stub will have lower maintenance costs for end users.

Refs: #29989 (comment)

PR-URL: #30153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2019
Avoid Runtime Deprecations when an alias or a stub/no-op will suffice.
An alias or stub will have lower maintenance costs for end users.

Refs: #29989 (comment)

PR-URL: #30153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants