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

Implement the ability to register custom watcher implementations #91

Merged
merged 5 commits into from
Oct 8, 2020

Conversation

michalt
Copy link
Contributor

@michalt michalt commented Sep 23, 2020

Currently to handle special file systems, we need to internally patch the package to create specialized Watchers. This change allows to dynamically register a Watcher factory that will take precedence over the default options. The applications could then "register" such factories as needed.

CC @davidmorgan

This allows registering a special-purpose factory that returns its own
`Watcher` implementation that will take precedence over the default
ones. The main motivation for this is handling of file systems that need
custom code to watch for changes.
@michalt michalt force-pushed the custom-watcher-factory branch from b1c8092 to 195a23d Compare September 24, 2020 07:08
Copy link
Contributor

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

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

Generally LG, I think you already noticed the failing tests :)

lib/src/custom_watcher_factory.dart Outdated Show resolved Hide resolved
lib/src/custom_watcher_factory.dart Outdated Show resolved Hide resolved
/// the built-in watchers.
///
/// Note that we will try [CustomWatcherFactory] one by one in the order they
/// were registered.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm nervous that this could end up with different behaviour based on startup order ... some ideas for making it stable:

  • Check by name order instead of register order :)
  • Or, always check all of them, and fail if multiple watchers claim the same path

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, all the options have some downsides, but perhaps the one that throws is better, since we can provide a clear error and so any debugging should be easier.

lib/src/custom_watcher_factory.dart Outdated Show resolved Hide resolved
lib/src/directory_watcher.dart Outdated Show resolved Hide resolved
@michalt
Copy link
Contributor Author

michalt commented Sep 24, 2020

Thanks! (I had to make another change to one of the tests to make it work across platforms, hopefully this time it'll work better ;)

@davidmorgan davidmorgan merged commit e488064 into dart-lang:master Oct 8, 2020
@natebosch
Copy link
Member

I've got some changes I'd like to make to the design here. Is there a doc or prototype CL you have started for using this?

I'd like to understand why you need the capability to unregister a custom watcher.

@natebosch
Copy link
Member

One other thing to note, anytime we modify a package that isn't already at a -dev version we should add a new -dev version with an appropriate version bump.

https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change

@michalt
Copy link
Contributor Author

michalt commented Oct 12, 2020

@natebosch Thanks for taking a look! I don't strictly need the ability to unregister a watcher, so I'd be ok with removing it if that's what you prefer.

mosuem pushed a commit to dart-lang/tools that referenced this pull request Dec 11, 2024
…actory

Implement the ability to register custom watcher implementations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants