fs: make FSWatcher.start private

[lholmquist]

• This is a semver-major change to rename the FSWatcher.start
  function to FSWatcher._start to make it private

Also see #29872

The motivation here is that it serves no purpose to the end user.
An instance of FSWatcher is returned when a user calls fs.watch,
which will call the start method. A user can't create an instance
of a FSWatcher directly. If the start method is called by a user
it is a noop since the watcher has already started. Calling start
after a watcher has closed is also a noop

@addaleax @cjihrig I went straight for changing the function to private, do we need an entry or anything in the deprecations.md?

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[addaleax]

I think you can also use a symbol instead of an underscore prefix (we’ve been doing that a lot more since symbols became available), and the tests for .start() can also be dropped completely imo

[lholmquist]

@addaleax good to know about the symbols. I don't suppose you have a link to that usage handy :)

[addaleax]

@lholmquist There is some info in https://github.com/nodejs/node/blob/master/doc/guides/using-symbols.md – a basic usage example would be:

const kStart = Symbol('kStart');

FSWatcher.prototype[kStart] = function() { ... };

const watcher = new FSWatcher();
watcher[kStart]();

[cjihrig]

LGTM with the suggested switch to a symbol.

[lholmquist]

I like the idea of using the Symbols, but i don't think it can be done in this case. The function that we are making private and the place where the call to start is made are in separate files.

I did try moving that call to the start function to be inside the FSWatcher function, so when new FSWatcher() is called, it would call start, but that didn't actually work

[addaleax]

@lholmquist You could add the symbol to the exports of lib/internal/fs/watchers.fs and use that in lib/fs.js – it’s an internal module, so it’s still hard to access the symbol from userland code.

[lholmquist]

@addaleax ok, i'll do that. wasn't sure if it was ok to add another export, but like you said, it is an internal module

[lholmquist]

I added a commit that is using a Symbol. The reason for naming it the way i did was becuase i think something similar will probably happen with the StatWatcher.start method, so i wanted to make sure there was that differentiation

I also removed the tests for FSWatcher.start

[BridgeAR]

What's the semverness here?

[lholmquist]

@BridgeAR semver-major because it is removing a public non-documented api #29872 (comment)

[nodejs-github-bot]

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

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2053/

[Trott]

Added to the 13.0.0 milestone, but if CITGM reveals issues, we can take it off.

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2053/

Results look good.

[Trott]

Landed in 7eacb74

[jasnell]

This really should have been a deprecation for the existing API

[Trott]

This really should have been a deprecation for the existing API

Not too late to revert and go that route if you feel strongly about it (and no opposition from @addaleax, @cjihrig, @BridgeAR...). This hasn't gone out in a release yet.

[jasnell]

A new pr that adds a deprecated alias is good.... I'll be opening one in a few minutes see: #29989