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

Do not watch vendor/ for changes. #693

Merged
merged 1 commit into from
May 12, 2014

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented May 12, 2014

This fixes the polling regression that was added in #562 (broccoli-bower was doing this internally).

On my rMBP this change takes a super simple app (https://github.com/rjackson/_____ember-cli-test) from ~9% CPU usage down to ~2% (during ember server).

@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2014

I believe that this resolves the performance issue that @krisselden mentioned recently.

@stefanpenner
Copy link
Contributor

does this also then not watch files explicitly added from vendor? e.g. app.import('vendor/foo/bar.js')

if so, we must find a solution for this, or we will have many sad plugin authors.

@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2014

app.import('vendor/foo/bar.js') will still work just fine, but if you change vendor/foo/bar.js you will need to restart your server.

Not sure why that would sadden folks, also this was the behavior prior to #562.

@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2014

I view vendor/ essentially as an analogue to node_modules/: it is generally not OK to edit those files manually. In addition we are specifically ignoring all of vendor/ via our .gitignore, so any changes/modifications made to those files would be lost upon next cloning.

@stefanpenner
Copy link
Contributor

@rjackson bower link/npm link are part of normal development workflows. When one makes such changes it is very important that we provide nice fast feedback.

It seems like files explicitly imported should be watched.

@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2014

@stefanpenner - When you run npm link do you expect a long running task that had previously started to get the updated package? I would assume not (as I do not know how that would even be possible). How is this different?

@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2014

I will think about how to get the explicitly added files watched.

@stefanpenner
Copy link
Contributor

I think the only solution is to watch files that are explicitly imported. I believe the issue, is when we end up watching much much much larger tree's.

But I believe this is very important for our addon story, both for authors and consumers.

@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2014

I have updated this PR to do as you have asked. It will now watch the directory containing the file being imported (there is no way currently to watch a specific file only). This still results in a much lower CPU consumption than master, and is definitely a win.

@stefanpenner
Copy link
Contributor

@rjackson any ideas to watch specific files?

@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2014

@stefanpenner - Yes, I am working on it, but it will have to be updated in Broccoli (working on refactoring the Broccoli::Watcher). This is still a pretty good quick win in the meantime.

stefanpenner added a commit that referenced this pull request May 12, 2014
Do not watch vendor/ for changes.
@stefanpenner stefanpenner merged commit 133c658 into ember-cli:master May 12, 2014
@stefanpenner
Copy link
Contributor

thanks @rjackson :)

@stefanpenner stefanpenner deleted the do-not-watch-vendor branch May 12, 2014 14:22
@tonywok
Copy link
Contributor

tonywok commented May 12, 2014

Nice! My poor little macbook air's fan thanks you. 👍

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