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

Provide a --watcher option to allow the user to specify watcher to use #970

Merged
merged 3 commits into from
Jun 11, 2014

Conversation

jlauemoeller
Copy link
Contributor

The events-based Sane watcher only works under the assumption that source changes are made on the same file system as that monitored by the watcher. While this is normally the case, the assumption fails when the watcher runs on a mapped version of the development file system, e.g. inside a Vagrant box.

In this setup, ember serve runs inside the virtual machine and sees a mapped version of the actual development folder on the host machine (e.g. the developer's mac). When the developer changes source files on the host file system, these changes are immediately visible inside the VM, but since the VM only sees a mapping of the files, it doesn't receive any related file system events. For this reason, the Sane watcher doesn't detect any changes.

This PR simply adds a --watcher option that allows the user to specify that the sane watcher should a polling strategy instead of the default events-based one. With this change in place, ember-cli once again works in Vagrant-like setups.

--watcher events chooses the events-based watcher strategy. This is the default when the option is not specified
--watcher polling chooses the polling watcher strategy.

@rwjblue
Copy link
Member

rwjblue commented Jun 10, 2014

I think generally, I would prefer the flag to be --polling and default to false.

@stefanpenner
Copy link
Contributor

I like: --watcher <arg> as it is self documenting

@rwjblue
Copy link
Member

rwjblue commented Jun 10, 2014

@stefanpenner - You win. 😄

@stefanpenner
Copy link
Contributor

actually, https://github.com/amasad/sane says it works with vagrant, merely enable: poll

I would prefer for us to go this path, as sane is well maintained.

Patches to github.com/krisselden/broccoli-sane-watcher may be needed

@jlauemoeller
Copy link
Contributor Author

Hmmm. My guess would be that it depends on the specific box provider chosen and how the mapping is done. I use the Parallels box and in this case it doesn't work.

@stefanpenner
Copy link
Contributor

@jlauemoeller merely a poll option must me passed, so your --watch can be used to make the configuration.

@jlauemoeller
Copy link
Contributor Author

Ahh, I see it now in the sane source. Let me try that solution instead of switching watcher entirely.

@jlauemoeller
Copy link
Contributor Author

Looks like this approach will require an update to broccoli-sane-watcher as well as that doesn't support passing options to sane.

@rwjblue
Copy link
Member

rwjblue commented Jun 10, 2014

Sounds good.

@jlauemoeller
Copy link
Contributor Author

Ok, I have a PR broccolijs/broccoli-sane-watcher#6 on broccoli-sane-watcher that enables passing the poll option on to sane. I will wait for someone to take a look at that.

@stefanpenner
Copy link
Contributor

@jlauemoeller i merged that, but don't have npm creds for it. I pinged @krisselden to release it, so we can get this in tonight yet for 0.0.34

@jlauemoeller
Copy link
Contributor Author

Thanks, that would be awesome.

@stefanpenner
Copy link
Contributor

@jlauemoeller can you rebase this and update to point to the git ref of broccoli-sane-watcher so that I can test it out easily?

@jlauemoeller
Copy link
Contributor Author

Yeah, I was just looking at that but I'm not sure how to add the git ref to package.json (new kid on the block). Simply adding the url#sha makes npm install fail

@stefanpenner
Copy link
Contributor

@jlauemoeller no need, @krisselden just gave me npm access, v0.0.4 is released. :)

If you can update accordingly, and rebase here we can merge. And include it into tonights release.

@jlauemoeller
Copy link
Contributor Author

Alright seems Travis CI is happy, please have a look

@rwjblue
Copy link
Member

rwjblue commented Jun 10, 2014

LGTM - 👍

stefanpenner added a commit that referenced this pull request Jun 11, 2014
Provide a --watcher option to allow the user to specify watcher to use
@stefanpenner stefanpenner merged commit 235a5c7 into ember-cli:master Jun 11, 2014
rwjblue added a commit that referenced this pull request Jun 11, 2014
simonexmachina added a commit to simonexmachina/ember-cli that referenced this pull request Jun 12, 2014
* 'master' of git://github.com/stefanpenner/ember-cli: (21 commits)
  Add smoke tests for default Brocfile.
  Use Project model from EmberApp.
  Ensure JS is concated safely.
  Add integration-test blueprint
  Update CHANGELOG.md
  release v0.0.34
  Add Brocfile test for `wrapInEval`.
  Do not wrap vendor in eval when `wrapInEval` is set.
  Add missing CHANGELOG entries.
  Fix whitespace.
  [fixes ] update es3 filter to v0.0.7
  update broccoli-sane-watcher to 0.0.5. It now should provided better errors when trying to watching something non-existent
  Add link for ember-cli#970 to CHANGELOG.
  inflection should not be a a depedency not a dev dependency
  improve conditional
  Smoke Test Refactor
  Update changelog
  Use poll flag for sane instead of switching between watchers
  Provide a --watcher option to allow the user to specify which watcher to use
  Make sure the build traps `SIGINT` and `SIGTERM` for cleanup.
  ...
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.

3 participants