fs: migrate fs_event_wrap to internal/errors

[maclover7]

Migrates fs_event_wrap.cc to internal/errors style.

Checklist

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

Affected core subsystem(s)

fs

[maclover7]

ping @TimothyGu

[targos]

@maclover7 #17682 landed. Can you please rebase?

[maclover7]

updated @targos

[maclover7]

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

[mcollina]

this is not the same, why this change?
It seems we are not exposing start  at all, see https://nodejs.org/api/fs.html#fs_class_fs_fswatcher.
I think we should add it to the docs.

[joyeecheung]

Looks like this should be watcher.start(testCase[testCase.field])?

[joyeecheung]

Looking at #17430 and #17432 I think this line should be either:

1. watcher.start(testCase[testCase.field]), we enforce that a filename must be passed to .start()
2. watcher.start(), and return silently in FSWatcher.prototype.start() if the filename is undefined (current behavior).

[joyeecheung]

Umm, looking more closely I think .start() is accepted the second time it is called (i.e. the wrap is already initialized to watch on a file), so in JS land we should check if the wrap is initialized before calling validatePath(). The easiest way to do it is to store a field in the FSWatcher.prototype.start() in JS land, echoing what's being done in the C++ land (it also needs to be updated in .close())

[joyeecheung]

Or, if we don't want to return silently when someone tries to call .start() again on the initialized watcher:

if (!isInitialized) {
  validatePath(filename);
} else {
  throw errors.Error('ERR_FS_EVENT_STARTED');  // A new Error indicating this has been started.
}

If we don't even want to document .start(), we can just assert(isInitialized) instead of creating a public error for it.

[mhdawson]

LGTM once comment about watcher.start() is addressed.

[maclover7]

updated @joyeecheung PTAL

[joyeecheung]

LGTM with an suggestion

[joyeecheung]

Can you add a comment here mentioning that this should be kept in sync with the initialized field of the C++ wrap? (Arguably better if this is an accessor property on the prototype of FSWatcher, that way there is no need to set this field in start and close, although that brings a few more calls into C++ so I am fine with this as well)

[joyeecheung]

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

[BridgeAR]

@maclover7 would you be so kind and rebase and also address the comment?

[BridgeAR]

Ping @maclover7

[BridgeAR]

Closing due to no further progress. @maclover7 please feel free to reopen in case you would like to continue working on this.