fs: add missed FSWatcher.prototype.start deprecation

[jasnell]

#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

[Trott]

Suggested change

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

[Trott]

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]

@nodejs/tsc

[lholmquist]

LGTM with the already suggested changes

[addaleax]

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]

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]

@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]

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]

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

[lholmquist]

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]

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]

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]

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

[cjihrig]

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

[ChALkeR]

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]

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]

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]

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

[Trott]

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)

[lholmquist]

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