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: make FSStatWatcher.start private #29971

Closed
wants to merge 4 commits into from

Conversation

lholmquist
Copy link
Contributor

Similar to #29905

An instance of FSStatWatcher is returned when a user calls fs.watchFile,
which will call the start method. A user can't create an instance
of a FSStatWatcher directly. If the start method is called by a user
it is a noop since the watcher has already started.

ATM, a user could call start on a closed watcher and i believe it would restart it, however, they would need to re-pass the filename and other function parameters. I haven't actually tested that out, though. This is what i see from the code. It would be nice if it acted in a similar fashion to the FSWatch class, and a noop would happen if start was called on a closed watcher, but if this function becomes private, then that wouldn't matter

This method is also undocumented, in fact, the whole "Class" FSStatWatcher seems to be undocumented, although that can be a different issue/PR

This PR makes this method private, and would be a semver-major change, similar to the link issue above

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 14, 2019
@jasnell
Copy link
Member

jasnell commented Oct 15, 2019

Unfortunately I think this will require a deprecation cycle before it can be made private

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 15, 2019
@lholmquist
Copy link
Contributor Author

@jasnell i'm good with whatever, but what is the difference between this PR and the previous PR i did, #29905, where there was no deprecation cycle? Both of these methods happen to be undocumented currently

@jasnell
Copy link
Member

jasnell commented Oct 15, 2019

That should have included a deprecation also. I hadn't seen it before it landed

@lholmquist
Copy link
Contributor Author

I see that new deprecation PR, #29989, for the FSWatcher.start. I'll do the same then for this one

@lholmquist
Copy link
Contributor Author

There seems to be some discussion happening in this issue, #29989 (comment), regarding the needing a deprecation or not, so going to wait to see what happens there before proceeding

@lholmquist
Copy link
Contributor Author

It looks like the outcome of #29989 (comment) , was that it is prefered to have an alias or stub than a runtime deprecation for this type of thing.

I'm going to update this PR then to do that

@lholmquist
Copy link
Contributor Author

Updated this PR with the stubbed out start method

@lholmquist
Copy link
Contributor Author

Also gave this a rebase since it was out of date

@lholmquist
Copy link
Contributor Author

also adding in a comment, which was suggested here: #30160 (review)

I also realize i have a 2 different PR's open for the same file, sorry about that :)

An instance of FSStatWatcher is returned when a user calls fs.watchFile,
which will call the start method. A user can't create an instance
of a FSStatWatcher directly. If the start method is called by a user
it is a noop since the watcher has already started.

This "Class" is currently undocumented
@lholmquist
Copy link
Contributor Author

Just did a rebase now that #30160 has been merged

@lholmquist
Copy link
Contributor Author

@jasnell @addaleax If you have time i would appreciate your feedback.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you for the ping!

@addaleax
Copy link
Member

This needs another @nodejs/tsc approval in order to land

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 18, 2019
@Trott
Copy link
Member

Trott commented Nov 18, 2019

addaleax pushed a commit that referenced this pull request Nov 19, 2019
An instance of FSStatWatcher is returned when a user calls fs.watchFile,
which will call the start method. A user can't create an instance
of a FSStatWatcher directly. If the start method is called by a user
it is a noop since the watcher has already started.

This "Class" is currently undocumented.

PR-URL: #29971
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@addaleax
Copy link
Member

Landed in 535e957 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

7 participants