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

feat(watcher): add a watcher for ibazel #2895

Closed
wants to merge 3 commits into from

Conversation

alexeagle
Copy link

ibazel (github.com/bazelbuild/bazel-watcher)
is a watch mode for Google's build tool, bazel
(http://bazel.build)
Under this watch mode, tools like Karma should not
directly watch .js files, because the build tool will
not write them all at once. Instead, the build tool
signals when it's time to reload the .js sources.

/cc @gregmagolan @achew22

ibazel (github.com/bazelbuild/bazel-watcher)
is a watch mode for Google's build tool, bazel
(http://bazel.build)
Under this watch mode, tools like Karma should not
directly watch .js files, because the build tool will
not write them all at once. Instead, the build tool
signals when it's time to reload the .js sources.
});
rl.on('close', () => {
// Give ibazel 5s to kill our process, otherwise do it ourselves
setTimeout(() => {
Copy link

Choose a reason for hiding this comment

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

I think this is unnecessary. If the stream is closed it will be because you're being killed and it is a reasonable response to just quit immediately.

@dignifiedquire
Copy link
Member

Not a big fan of adding very tool specific support into core. I would rather see a plugable way to support different watching strategies through plugins. Also this adds the burden of maintenance of a tool I have no experience with or use for at the moment.

@alexeagle
Copy link
Author

@dignifiedquire I understand that motivation. At the same time, AFAICT there isn't a way for a plugin to extend the watchers right now. Have there been any other requests to add a different file watcher? I could imagine doing the work to add this ability to the plugin system, only to see this be the last plugin that ever uses that ability.
Do you want me to try to add such a thing? My other options are to have Bazel users depend on a fork, or find some way to monkey-patch this at load time.

@dignifiedquire
Copy link
Member

I think I would be fine keeping the code in here, but lets restructure things a little more cleanly so it is clear that there are multiple ways to watch files.

For this to be ready to merge I would like to see

  • documentation on how to integrate with bazel
  • a new folder lib/watchers, which holds the current watcher, which I would call filesystem watcher for now and the new bazel watcher
  • unit tests for the bazel watcher
  • ideally an e2e test that uses the bazel watcher and makes sure it works

@kyliau kyliau force-pushed the ibazel branch 5 times, most recently from 770a3d2 to fa1a84a Compare January 18, 2019 01:31
kyliau pushed a commit to kyliau/karma that referenced this pull request Jan 19, 2019
Over the years, there has been several suggestions to make the default
watcher in Karma (that uses chokidar) dependency injectable so that
Karma could work well with third-party watchers.

Prior issues:
1. karma-runner#1468
2. karma-runner#2895

Candidate third-party watchers include:
1. Watcher for Broccoli
2. Watcher for Bazel
3. Watcher for Blaze (used internally at Google)

This commit makes the change to allow other Karma plugins to specify the
'watcher' value in the di system and Karma would use the supplied
watcher instead.
johnjbarton pushed a commit that referenced this pull request Jan 22, 2019
…ers (#3254)

Over the years, there has been several suggestions to make the default
watcher in Karma (that uses chokidar) dependency injectable so that
Karma could work well with third-party watchers.

Prior issues:
1. #1468
2. #2895

Candidate third-party watchers include:
1. Watcher for Broccoli
2. Watcher for Bazel
3. Watcher for Blaze (used internally at Google)

This commit makes the change to allow other Karma plugins to specify the
'watcher' value in the di system and Karma would use the supplied
watcher instead.
@johnjbarton
Copy link
Contributor

Watchers are now configurable.

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

Successfully merging this pull request may close these issues.

4 participants