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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filesystem watcher API #14853

Merged
merged 97 commits into from Aug 8, 2017

Conversation

Projects
None yet
6 participants
@smashwilson
Member

smashwilson commented Jun 20, 2017

This will add a central filesystem watcher to Atom's core API, so that multiple packages can share the same native filesystem watchers, conserving operation system resources.

There are a ton of Node filesystem watcher packages with varying degrees of cross-platform compatibility, error resilience, and sophistication of configuration. To keep our scope from creeping uncontrollably, the first version we ship will target only the minimum functionality that we need for bundled core packages and @damieng's language server integration.

See the original discussion in #14163.

API

This will add two public methods to subscribe to filesystem changes, an "extended" general-purpose one to recursively watch any directory tree, and a "public" specialized one to watch for changes within project roots that are open within atom.projects.

The general-purpose one will involve calling watchPath instances directly, by means of a function exported from the atom require:

const {watchPath} = require('atom')

const watcher = watchPath(rootPath, {}, events => {
  // Accept batch of events
})

const disposable1 = watcher.onDidError(err => {
  // Handle error
})

// After this promise resolves, changes made to files on the filesystem will result in
// the registered callback(s) being invoked
await watcher.getStartPromise()

// After this promise resolves, all OS watcher resources will be disposed
await watcher.dispose()
disposable1.dispose()
  • rootPath can be a path anywhere on the filesystem.
  • If rootPath is a directory, its contents will be watched recursively. If it's an individual file, only that file will be watched.
  • The PathWatcher instances returned by watchPath() may be backed by an existing native filesystem watcher on the same root path or a parent directory.
  • The native watcher may also be swapped out if a watcher is started on a parent directory, or if its native watcher is assigned to a parent directory but the parent watcher is disposed. All of these changes are transparent to consumers.
  • getStartPromise() is useful to have for writing test cases - you want to be sure the watcher is initialized and receiving events before manipulating the filesystem. It will not be necessary to wait for the watcher to start before subscribing to events, though.
const disposable = atom.project.onDidChangeFiles(events => {
  // ..
})
  • This API will be implemented with the other. Notably, watchers created here will participate in the deduping process described above.
  • As new roots are added to the project or removed, existing event subscriptions will be adjusted accordingly.

In both cases, the payload delivered to the change callback is an Array with the following structure:

[
  {
    "action": "", // one of "modified", "created", "deleted", or "renamed"
    "oldPath": "", // undefined unless type is "renamed," in which case it holds the former name of the entry
    "path": "" // absolute path to the changed entry
  }
]

馃棐 I'm writing this initial cut with nsfw (which we use in the GitHub package). I'm planning to merge this PR still using nsfw and cutting atom/github over to using it as part of the next release so we're no worse off in stable than we are today.

After 馃殺, I'll replace it with a bespoke native watching module. nsfw launches two threads for each watched directory root, which makes me nervous to use it to back a user-facing API.

Remaining Work

  • Create a NativeWatcherRegistry to manage the attachment of native watchers to userland watchers.
    • Re-use native watchers for exact directories
    • Re-use existing native watchers on parent directories
    • Replace native watchers on child directories with a new watcher on a parent directory
    • Remove unused native watchers
    • Re-watch individual child directories when a parent directory's watcher is removed
    • Documentation complete
    • Test coverage
      • More complicated watcher splits: parent-child relationships among the child watchers
  • Implement a NativeWatcher to translate native watcher library events to match the spec 鈽濓笍
    • Rebroadcast change events through an event-kit Emitter
    • Broadcast an event when event consumers should attach themselves to a new NativeWatcher instead
    • Handle errors... somehow
    • Begin watching lazily when the first change subscriber is registered
    • Automatically shut down when no subscribers exist
  • Create a PathWatcher object and watchPath function to supply the user-facing watcher API
    • Broadcast onDidStart when attached to a native watcher that has begun listening for events
    • Rebroadcast onDidChange events to subscribers
    • Change onDidStart to the more useful getStartPromise
    • Lazily create onDidChange subscription
    • Documentation complete
  • Re-export the watchPath function from "atom"
    • Documentation
  • Implement atom.project.onDidChangeFiles() using PathWatchers
    • Tests
    • Documentation
  • Take it for a final spin
  • Implement a feature flag
  • Verify that the docs look okay in Joanna
  • Ensure CI is green
Show outdated Hide outdated src/filesystem-manager.js
@sadovnychyi

This comment has been minimized.

Show comment
Hide comment
@sadovnychyi

sadovnychyi Jun 27, 2017

Seems great, but how could I ignore files ignored by Atom? Specifically globs in "Ignored Names" and "Exclude VCS Ignored Paths" core settings. It would be very helpful to have an option to ignore them automatically.

sadovnychyi commented Jun 27, 2017

Seems great, but how could I ignore files ignored by Atom? Specifically globs in "Ignored Names" and "Exclude VCS Ignored Paths" core settings. It would be very helpful to have an option to ignore them automatically.

@sadovnychyi sadovnychyi referenced this pull request Jun 27, 2017

Open

[WIP] Complete CSS selectors inside of attributes #21

2 of 7 tasks complete
@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 28, 2017

Thanks for this! Main reason I've been procrastinating with developing atom-fs is a juggling act between existing event listeners. Being able to reliably respond to changes will make things a hell of a lot easier. =) Kudos!

Alhadis commented Jun 28, 2017

Thanks for this! Main reason I've been procrastinating with developing atom-fs is a juggling act between existing event listeners. Being able to reliably respond to changes will make things a hell of a lot easier. =) Kudos!

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 10, 2017

Member

@damieng: Now that I'm looking at this again, I think I prefer the original form of the API instead:

atom.filesystem.getWatcher('/path/to/root').onDidChange(changes => {
  // ...
})

I think it feels a bit too odd to me to use the argument to .for(...) as the root of the watcher returned by a different method. 馃

Eventually, we can:

  • Add a atom.filesystem method to register a backing implementation for a specific subtree or path prefix; and
  • use the path argument to getWatcher() to dispatch internally to a specific filesystem implementation based on the root tree of the watcher.

Seems great, but how could I ignore files ignored by Atom? Specifically globs in "Ignored Names" and "Exclude VCS Ignored Paths" core settings. It would be very helpful to have an option to ignore them automatically.

@sadovnychyi: Good point! I think an exclusion list would be in-scope, as at least the tree-view could use it. It's especially important on Linux, too, where inotify handles can be a scarce resource. And, I like the idea of allowing users to inherit exclusions from the core settings.

Note that the file-watching package we're using here doesn't support exclusions natively; but I'm already planning on rolling our own so I can just support it there 馃槃

A few considerations and complications that this introduces:

  1. Exclusions need to be accounted for when backing multiple Watcher instances by a single NativeWatcher instance. Specifically, each NativeWatcher can only exclude the intersection of the paths excluded by all attached Watchers.
  2. What's the most graceful way to specify watcher exclusions? Chokidar-style anymatch patterns? minimatch globs? Note that we need to be able to do basic set operations on patterns for point (1).
  3. We'll need to add-and-replace NativeWatchers as new Watchers are attached with different exclusion lists. Fortunately we already do this for the split operation, so the machinery is all there.
  4. How do users request core setting exclusions? Adding exported Symbols to an array?
  5. If a Watcher is bound to a core setting exclusion, it'll also need to update itself if that setting is changed.

Thanks for this! Main reason I've been procrastinating with developing atom-fs is a juggling act between existing event listeners. Being able to reliably respond to changes will make things a hell of a lot easier. =) Kudos!

鉂わ笍

Do you have any opinions about other capabilities and options that you'd need in a native API?

Member

smashwilson commented Jul 10, 2017

@damieng: Now that I'm looking at this again, I think I prefer the original form of the API instead:

atom.filesystem.getWatcher('/path/to/root').onDidChange(changes => {
  // ...
})

I think it feels a bit too odd to me to use the argument to .for(...) as the root of the watcher returned by a different method. 馃

Eventually, we can:

  • Add a atom.filesystem method to register a backing implementation for a specific subtree or path prefix; and
  • use the path argument to getWatcher() to dispatch internally to a specific filesystem implementation based on the root tree of the watcher.

Seems great, but how could I ignore files ignored by Atom? Specifically globs in "Ignored Names" and "Exclude VCS Ignored Paths" core settings. It would be very helpful to have an option to ignore them automatically.

@sadovnychyi: Good point! I think an exclusion list would be in-scope, as at least the tree-view could use it. It's especially important on Linux, too, where inotify handles can be a scarce resource. And, I like the idea of allowing users to inherit exclusions from the core settings.

Note that the file-watching package we're using here doesn't support exclusions natively; but I'm already planning on rolling our own so I can just support it there 馃槃

A few considerations and complications that this introduces:

  1. Exclusions need to be accounted for when backing multiple Watcher instances by a single NativeWatcher instance. Specifically, each NativeWatcher can only exclude the intersection of the paths excluded by all attached Watchers.
  2. What's the most graceful way to specify watcher exclusions? Chokidar-style anymatch patterns? minimatch globs? Note that we need to be able to do basic set operations on patterns for point (1).
  3. We'll need to add-and-replace NativeWatchers as new Watchers are attached with different exclusion lists. Fortunately we already do this for the split operation, so the machinery is all there.
  4. How do users request core setting exclusions? Adding exported Symbols to an array?
  5. If a Watcher is bound to a core setting exclusion, it'll also need to update itself if that setting is changed.

Thanks for this! Main reason I've been procrastinating with developing atom-fs is a juggling act between existing event listeners. Being able to reliably respond to changes will make things a hell of a lot easier. =) Kudos!

鉂わ笍

Do you have any opinions about other capabilities and options that you'd need in a native API?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jul 10, 2017

Do you have any opinions about other capabilities and options that you'd need in a native API?

Nope, none I can think of off the top of my head. =) Although, I hope I won't be seeing another File/Directory implementation class added later on... there were two of each,IIRC. One pair defined by the tree-view package, and another defined in the old Pathwatcher code... hectic. :(

Once again, great job! 鉁岋笍

Alhadis commented Jul 10, 2017

Do you have any opinions about other capabilities and options that you'd need in a native API?

Nope, none I can think of off the top of my head. =) Although, I hope I won't be seeing another File/Directory implementation class added later on... there were two of each,IIRC. One pair defined by the tree-view package, and another defined in the old Pathwatcher code... hectic. :(

Once again, great job! 鉁岋笍

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Jul 11, 2017

Contributor

I still think providing .for('path') giving us a filesystem object for that particular filesystem (e.g. local, remote etc) that has the necessary api's on it such as getWatcher is cleaner both in terms of use and implementation.

Otherwise we'll end up with a handful of methods that all take paths and that all have to have logic to go off and do the right thing per-filesystem. Even using it for a few things on a single folder feels cleaner, e.g.

atom.filesystem.getWatcher('/a/b/c')
atom.filesystem.getFolders('/a/b/c')
atom.filesystem.getFiles('/a/b/c')

vs

root = atom.filesystem.for('/a/b/c')
root.getFolders()
root.getFiles()
root.getWatcher()
Contributor

damieng commented Jul 11, 2017

I still think providing .for('path') giving us a filesystem object for that particular filesystem (e.g. local, remote etc) that has the necessary api's on it such as getWatcher is cleaner both in terms of use and implementation.

Otherwise we'll end up with a handful of methods that all take paths and that all have to have logic to go off and do the right thing per-filesystem. Even using it for a few things on a single folder feels cleaner, e.g.

atom.filesystem.getWatcher('/a/b/c')
atom.filesystem.getFolders('/a/b/c')
atom.filesystem.getFiles('/a/b/c')

vs

root = atom.filesystem.for('/a/b/c')
root.getFolders()
root.getFiles()
root.getWatcher()
@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 11, 2017

Member

@damieng: I was thinking that I'd hide pretty much exactly that pattern internally. Something like:

class NativeFileSystem {
  getWatcher (root) {
    // ...
  }
}

class RemoteFileSystem {
  getWatcher (root) {
    // ...
  }
}

export default class FileSystemManager {
  getWatcher (root) {
    return this.for(root).getWatcher(root)
  }

  register (root, impl) {
    // Handle any paths under "root" with the implementation "impl".
  }

  for (root) {
    // Detect the appropriate FileSystem implementation registered for the root path.
    // Maybe automatically detect filesystem capabilities somehow?
  }
}

I think what bugs me most about the .for(...).getWatcher() approach is that the filesystem object returned from .for() needs to remember the "anchor point" that you queried to access it, even though it conceptually represents the behavior of an entire mounted filesystem. It would be weird to me to have two instances of NativeFileSystem that represent the same physical disk, but that aren't === and have different behavior for argumentless getters called later.

I could even tidy up the duplication in that sample with a bit of metaprogramming:

function delegateToImpl (proto, methodName) {
  proto[methodName] = function (pathArg, ...args) {
    return this.for(pathArg)[methodName](...args)
  }
}

for (const methodName of ['getWatcher', 'getFolders', 'getFiles']) {
  delegateToImpl(FileSystemManager.prototype, methodName)
}

... although that would break our documentation something fierce 馃槈

馃 Maybe it's just the FooFileSystem names that I'm getting hung up on. Is there a better name to communicate what each instance actually represents? I'm with @Alhadis in that I really don't want to have another Directory class 馃槀

Member

smashwilson commented Jul 11, 2017

@damieng: I was thinking that I'd hide pretty much exactly that pattern internally. Something like:

class NativeFileSystem {
  getWatcher (root) {
    // ...
  }
}

class RemoteFileSystem {
  getWatcher (root) {
    // ...
  }
}

export default class FileSystemManager {
  getWatcher (root) {
    return this.for(root).getWatcher(root)
  }

  register (root, impl) {
    // Handle any paths under "root" with the implementation "impl".
  }

  for (root) {
    // Detect the appropriate FileSystem implementation registered for the root path.
    // Maybe automatically detect filesystem capabilities somehow?
  }
}

I think what bugs me most about the .for(...).getWatcher() approach is that the filesystem object returned from .for() needs to remember the "anchor point" that you queried to access it, even though it conceptually represents the behavior of an entire mounted filesystem. It would be weird to me to have two instances of NativeFileSystem that represent the same physical disk, but that aren't === and have different behavior for argumentless getters called later.

I could even tidy up the duplication in that sample with a bit of metaprogramming:

function delegateToImpl (proto, methodName) {
  proto[methodName] = function (pathArg, ...args) {
    return this.for(pathArg)[methodName](...args)
  }
}

for (const methodName of ['getWatcher', 'getFolders', 'getFiles']) {
  delegateToImpl(FileSystemManager.prototype, methodName)
}

... although that would break our documentation something fierce 馃槈

馃 Maybe it's just the FooFileSystem names that I'm getting hung up on. Is there a better name to communicate what each instance actually represents? I'm with @Alhadis in that I really don't want to have another Directory class 馃槀

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jul 11, 2017

Anybody here with the file-icons package installed and running, please open your dev-consoles and type the following line:

AtomFS

This is a globalised pointer to the singleton FileSystem class that handles all indexing, arrangement and disambiguation of entries. The name "FileSystem" has nothing to do with actual filesystems (the mountable/unmountable sorts, I mean).

It would be weird to me to have two instances of NativeFileSystem that represent the same physical disk, but that aren't === and have different behavior for argumentless getters called later.

Alhadis commented Jul 11, 2017

Anybody here with the file-icons package installed and running, please open your dev-consoles and type the following line:

AtomFS

This is a globalised pointer to the singleton FileSystem class that handles all indexing, arrangement and disambiguation of entries. The name "FileSystem" has nothing to do with actual filesystems (the mountable/unmountable sorts, I mean).

It would be weird to me to have two instances of NativeFileSystem that represent the same physical disk, but that aren't === and have different behavior for argumentless getters called later.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jul 11, 2017

I accidentally published that comment too soon, so rather than silently append extra details to my above post, I'll just fearlessly double-post instead. 馃憤

It would be weird to me to have two instances of NativeFileSystem that represent the same physical disk

See, the way atom-fs does it is that it returns old instances pointing to previously created ones if it's determined that two disk entities are logically identical.

If you glance through the module's code now, you'll notice it's sparse of documentation or any advanced filesystem logic. I'm looking forward to changing that once this troff/PDF renderer is finished.

Alhadis commented Jul 11, 2017

I accidentally published that comment too soon, so rather than silently append extra details to my above post, I'll just fearlessly double-post instead. 馃憤

It would be weird to me to have two instances of NativeFileSystem that represent the same physical disk

See, the way atom-fs does it is that it returns old instances pointing to previously created ones if it's determined that two disk entities are logically identical.

If you glance through the module's code now, you'll notice it's sparse of documentation or any advanced filesystem logic. I'm looking forward to changing that once this troff/PDF renderer is finished.

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2017

Member

馃 Maybe it's just the FooFileSystem names that I'm getting hung up on. Is there a better name to communicate what each instance actually represents?

Thinking about this some more, I think what I really want is a different set of names for things. How about:

atom.filesystem.at('/root/path').getWatcher()

Still need to think of a better name for "the thing returned by .at()."

@Alhadis, I'll have a look at atom-fs to see how you've done things tomorrow 馃槃 I think the Nuclide team has some prior art here, too.

Part of the problem is that we're trying to design two new public APIs at once here: the watcher API and a larger, theoretical pluggable-filesystem API, that we don't know the full scope of yet. I'd spent some time at the outset looking around at how other filesystem watcher APIs were designed, but I didn't look at the pluggable filesystem side at all.

Member

smashwilson commented Jul 12, 2017

馃 Maybe it's just the FooFileSystem names that I'm getting hung up on. Is there a better name to communicate what each instance actually represents?

Thinking about this some more, I think what I really want is a different set of names for things. How about:

atom.filesystem.at('/root/path').getWatcher()

Still need to think of a better name for "the thing returned by .at()."

@Alhadis, I'll have a look at atom-fs to see how you've done things tomorrow 馃槃 I think the Nuclide team has some prior art here, too.

Part of the problem is that we're trying to design two new public APIs at once here: the watcher API and a larger, theoretical pluggable-filesystem API, that we don't know the full scope of yet. I'd spent some time at the outset looking around at how other filesystem watcher APIs were designed, but I didn't look at the pluggable filesystem side at all.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jul 12, 2017

鈥 the watcher API and a larger, theoretical pluggable-filesystem API, that we don't know the full scope of yet. I'd spent some time at the outset looking around at how other filesystem watcher APIs were designed, but I didn't look at the pluggable filesystem side at all.

Haha, the filesystem API I developed was specifically designed to be a pluggable, dev-friendly interaction layer that made querying basic stats simple while being maximally conservative on system resources.

However, filewatching isn't its responsibility. It can monitor files, but at the moment, it's simply using the old File/Directory-type pathwatchers. I feel what Atom needs most is a focussed, explicit filewatching mechanism that would both simplify common IO tasks, and also complement the atom-fs package itself...

EDIT: Hope that made sense. 馃槩

Alhadis commented Jul 12, 2017

鈥 the watcher API and a larger, theoretical pluggable-filesystem API, that we don't know the full scope of yet. I'd spent some time at the outset looking around at how other filesystem watcher APIs were designed, but I didn't look at the pluggable filesystem side at all.

Haha, the filesystem API I developed was specifically designed to be a pluggable, dev-friendly interaction layer that made querying basic stats simple while being maximally conservative on system resources.

However, filewatching isn't its responsibility. It can monitor files, but at the moment, it's simply using the old File/Directory-type pathwatchers. I feel what Atom needs most is a focussed, explicit filewatching mechanism that would both simplify common IO tasks, and also complement the atom-fs package itself...

EDIT: Hope that made sense. 馃槩

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jul 12, 2017

What's wrong with this?

// Returns new PathWatcher or WatchEverYouWannaCallIt
let wat = atom.pathWatchers.watchFile("/path/to/whatever");

// Requesting an identical watcher returns reference to an existing watcher;
let wat2 = atom.pathWatchers.watchFile("/path/to/whatever");
wat === wat2; // true

// Etc.
let folder = atom.pathWatchers.watchDirectory("/path/to/");

Alhadis commented Jul 12, 2017

What's wrong with this?

// Returns new PathWatcher or WatchEverYouWannaCallIt
let wat = atom.pathWatchers.watchFile("/path/to/whatever");

// Requesting an identical watcher returns reference to an existing watcher;
let wat2 = atom.pathWatchers.watchFile("/path/to/whatever");
wat === wat2; // true

// Etc.
let folder = atom.pathWatchers.watchDirectory("/path/to/");
@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 31, 2017

Member

What's wrong with this?

That's fairly close to the atom.watchers global I started with. I like "pathWatchers" better than "watchers" because it answers the question "watching what?" but I'm less fond of watchFile and watchDirectory as separate entry points because it's easy to distinguish files and directories within the watcher logic (since we have to resolve symlinks and do stat() calls anyway). There is also asynchronicity to address; filesystem events will not be delivered to the callback until the native watcher has been started, which is a background task, which is important to know when you're writing tests that start watchers then touch files and expect the events to arrive.

But: we had some discussions and group-bikeshedding about this as a team last week, and a few other points came up:

  • The other objects attached to the atom global are things that manipulate global application state: the workspace, the view registry. While the filesystem watchers are technically stateful because of the de-duplication logic, consumers of this API are insulated from that.
  • Pluggable filesystem backends are not on our immediate roadmap, and shipping atom.filesystem or atom.fs with a single method would make it look like they were. It would also cause a certain amount of lock-in to a given API shape when and if we do get to that when we aren't ready to design it out just yet.
  • On the other hand, if we do plan to move in that direction, using it here would keep us from needing to do a round of deprecations when we introduce it.

Option 1

Re-export the Watcher object directly from the atom require:

const {Watcher} = require('atom')

const w = new Watcher(rootPath, {})

const sub0 = w.onDidChange(events => {
  // Batched filesystem events received
})

const sub1 = w.onDidError(err => {
  // An error happened
})

// Resolves when the native watcher is in place and events will be delivered
await w.getStartPromise()

sub0.dispose()
sub1.dispose()

// Resolves when the native watcher has been successfully terminated and its resources
// released
await w.getStopPromise()
  • 馃憤 No atom.filesystem or atom.fs API to try to get right now, before we're ready to tackle it properly.
  • 馃憤 Separate event and error subscriptions, managed by Atom-y event-kit subscriptions.
  • 馃憤 Easy to re-export from atom/sfw once Watcher and the native watcher registry are moved there (which is consistent with the way some other library objects are re-exported).
  • 馃憥 Complicated lifecycle promises to track internal state. Getting these wrong has been a common source of flaky tests on atom/github.
  • 馃憥 Still a relatively large public API surface to expose.

Option 2

const {watchPath} = require('atom')

const disposable = await watchPath(rootPath, {}, (err, events) => {
  // Handle error or batched events
})

await disposable.dispose()
  • 馃憤 The most minimal addition to the public API.
  • 馃憤 It's harder to mess up the watcher lifecycle. Events will be delivered as soon as the returned Promise resolves.
  • 馃憥 Breaks with Atom convention in two ways: (a) It would be the first function exported directly from require('atom') (b) We tend not to use error-receiving callbacks elsewhere. It's much more "node-y" than "Atom-y".
  • 馃憥 Performance hit. There's most often no reason to need to yield the event loop to wait for the watcher to be fully started unless you're writing test cases; it's perfectly reasonable to start a bunch of other async operations without waiting for the callback to be "live".

Event Schema

"oldPath" and "newPath" are confusing. But, we want to have paths at consistent keys from event to event, to avoid having to probe for multiple paths when determining if a batch of events touch certain files or not. We could either add a redundant "path" key or use "oldPath" and "path" for rename events. I'm leaning toward using "oldPath" and "path".

Summary

The bottom line, I think, is "stop agonizing over this, @smashwilson, and ship something." After some reflection over the weekend, I'm leaning toward pursuing option 1. Unless someone chimes in with strong objections before I'm finished, that's what I'll pursue 馃槃

Member

smashwilson commented Jul 31, 2017

What's wrong with this?

That's fairly close to the atom.watchers global I started with. I like "pathWatchers" better than "watchers" because it answers the question "watching what?" but I'm less fond of watchFile and watchDirectory as separate entry points because it's easy to distinguish files and directories within the watcher logic (since we have to resolve symlinks and do stat() calls anyway). There is also asynchronicity to address; filesystem events will not be delivered to the callback until the native watcher has been started, which is a background task, which is important to know when you're writing tests that start watchers then touch files and expect the events to arrive.

But: we had some discussions and group-bikeshedding about this as a team last week, and a few other points came up:

  • The other objects attached to the atom global are things that manipulate global application state: the workspace, the view registry. While the filesystem watchers are technically stateful because of the de-duplication logic, consumers of this API are insulated from that.
  • Pluggable filesystem backends are not on our immediate roadmap, and shipping atom.filesystem or atom.fs with a single method would make it look like they were. It would also cause a certain amount of lock-in to a given API shape when and if we do get to that when we aren't ready to design it out just yet.
  • On the other hand, if we do plan to move in that direction, using it here would keep us from needing to do a round of deprecations when we introduce it.

Option 1

Re-export the Watcher object directly from the atom require:

const {Watcher} = require('atom')

const w = new Watcher(rootPath, {})

const sub0 = w.onDidChange(events => {
  // Batched filesystem events received
})

const sub1 = w.onDidError(err => {
  // An error happened
})

// Resolves when the native watcher is in place and events will be delivered
await w.getStartPromise()

sub0.dispose()
sub1.dispose()

// Resolves when the native watcher has been successfully terminated and its resources
// released
await w.getStopPromise()
  • 馃憤 No atom.filesystem or atom.fs API to try to get right now, before we're ready to tackle it properly.
  • 馃憤 Separate event and error subscriptions, managed by Atom-y event-kit subscriptions.
  • 馃憤 Easy to re-export from atom/sfw once Watcher and the native watcher registry are moved there (which is consistent with the way some other library objects are re-exported).
  • 馃憥 Complicated lifecycle promises to track internal state. Getting these wrong has been a common source of flaky tests on atom/github.
  • 馃憥 Still a relatively large public API surface to expose.

Option 2

const {watchPath} = require('atom')

const disposable = await watchPath(rootPath, {}, (err, events) => {
  // Handle error or batched events
})

await disposable.dispose()
  • 馃憤 The most minimal addition to the public API.
  • 馃憤 It's harder to mess up the watcher lifecycle. Events will be delivered as soon as the returned Promise resolves.
  • 馃憥 Breaks with Atom convention in two ways: (a) It would be the first function exported directly from require('atom') (b) We tend not to use error-receiving callbacks elsewhere. It's much more "node-y" than "Atom-y".
  • 馃憥 Performance hit. There's most often no reason to need to yield the event loop to wait for the watcher to be fully started unless you're writing test cases; it's perfectly reasonable to start a bunch of other async operations without waiting for the callback to be "live".

Event Schema

"oldPath" and "newPath" are confusing. But, we want to have paths at consistent keys from event to event, to avoid having to probe for multiple paths when determining if a batch of events touch certain files or not. We could either add a redundant "path" key or use "oldPath" and "path" for rename events. I'm leaning toward using "oldPath" and "path".

Summary

The bottom line, I think, is "stop agonizing over this, @smashwilson, and ship something." After some reflection over the weekend, I'm leaning toward pursuing option 1. Unless someone chimes in with strong objections before I'm finished, that's what I'll pursue 馃槃

smashwilson added some commits Jun 16, 2017

smashwilson added some commits Aug 3, 2017

Show outdated Hide outdated script/test

smashwilson added some commits Aug 3, 2017

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 8, 2017

Member

I'm going to merge this to give @damieng something to work with and to give this as much time to percolate its way through master and beta as possible. This is a fairly high-risk change and I want to have as much time to surface issues as possible.

I've edited the PR description 鈽濓笍 with the changes that I've made to the API as I've gone, but to quickly summarize what's different, what's in, and what's out:

  • The two entry points are atom.project.onDidChangeFiles and require('atom').watchPath().
  • In the event spec:
    • type has become action. A near-term improvement is to be able to distinguish events involving files, directories, symlinks, and so forth and type felt ambiguous with that information.
    • newPath has become path to be less confusing in the common case of non-rename events. oldPath remains oldPath when present.
    • "added" has become "created" for better symmetry with "deleted". action values have also been lowercased for consistency with other node.js and Atom strings.
  • Exclusion lists are not accounted for with this PR, mainly because nsfw doesn't support them. I hope to add them to the new native backend and extend the options parameter to account for them in the future.
Member

smashwilson commented Aug 8, 2017

I'm going to merge this to give @damieng something to work with and to give this as much time to percolate its way through master and beta as possible. This is a fairly high-risk change and I want to have as much time to surface issues as possible.

I've edited the PR description 鈽濓笍 with the changes that I've made to the API as I've gone, but to quickly summarize what's different, what's in, and what's out:

  • The two entry points are atom.project.onDidChangeFiles and require('atom').watchPath().
  • In the event spec:
    • type has become action. A near-term improvement is to be able to distinguish events involving files, directories, symlinks, and so forth and type felt ambiguous with that information.
    • newPath has become path to be less confusing in the common case of non-rename events. oldPath remains oldPath when present.
    • "added" has become "created" for better symmetry with "deleted". action values have also been lowercased for consistency with other node.js and Atom strings.
  • Exclusion lists are not accounted for with this PR, mainly because nsfw doesn't support them. I hope to add them to the new native backend and extend the options parameter to account for them in the future.

@smashwilson smashwilson merged commit fb0e29c into master Aug 8, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw-filewatcher branch Aug 8, 2017

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 8, 2017

Member

@rsese, @lee-dohm: Just FYI, with this change disabling the GitHub package won't be enough to deal with nsfw-related crashes or lockups on 1.20-dev and up. Instead, I've added a core setting that fakes the API with already-existing Atom APIs:

screen shot 2017-08-08 at 2 09 26 pm

Member

smashwilson commented Aug 8, 2017

@rsese, @lee-dohm: Just FYI, with this change disabling the GitHub package won't be enough to deal with nsfw-related crashes or lockups on 1.20-dev and up. Instead, I've added a core setting that fakes the API with already-existing Atom APIs:

screen shot 2017-08-08 at 2 09 26 pm

@rsese

This comment has been minimized.

Show comment
Hide comment
@rsese

rsese Aug 8, 2017

Member

Thanks @smashwilson 鉁岋笍

/cc @Ben3eeE @50Wliu too as a heads up 鈽濓笍

Member

rsese commented Aug 8, 2017

Thanks @smashwilson 鉁岋笍

/cc @Ben3eeE @50Wliu too as a heads up 鈽濓笍

@steelbrain steelbrain referenced this pull request Aug 21, 2017

Open

[WIP] Project wide linting #1525

0 of 7 tasks complete

@avaly avaly referenced this pull request Oct 1, 2017

Merged

PathsCache with `find` #165

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment