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

Include resolved paths in watched paths #816

Merged
merged 4 commits into from
Dec 15, 2017

Conversation

maschwenk
Copy link
Contributor

@maschwenk maschwenk commented Sep 15, 2017

For people who are using resolved paths configurations, it might make sense to include these in the default_watched_paths. It won't affect anyone who left the resolved paths [] and it'll make test env re-compilation work for those who added resolved paths.

For folks who still have assets under app/assets/javascripts/, this will result in less hair pulling and fewer surprises. However, totally understand if you think this is better handled by an override like:

Webpacker::Compiler.watched_paths << 'app/assets/javascripts/**/*'

Thanks!

@maschwenk
Copy link
Contributor Author

@gauravtiwari you wrote the original implementation of that watch list, what do you think about this?

@gauravtiwari
Copy link
Member

@maschwenk Thanks for this but looking at the changes I am not very clear. Could you please explain the usage? Do you want resolved paths to be available through webpacker.yml instead?

@maschwenk
Copy link
Contributor Author

In #539 you setup this watch list so that in the test environment, it would lazily compile changed assets. This works great, but for people who have resolved_paths defined in their webpacker.yml, they most likely want those watched as well or else this feature won't work. The default webpacker.yml has:

resolved_paths: []

so for most people, my change will not affect them at all

@maschwenk maschwenk force-pushed the default-watch-include-resolved branch from be60d68 to be894a5 Compare September 18, 2017 20:18
@gauravtiwari
Copy link
Member

gauravtiwari commented Sep 18, 2017

@maschwenk Ahh gotcha, thanks. I think earlier, we didn't want to use webpacker.yml for ruby bits hence, explicit accessor to add watched paths but I think later we forgot to deprecate it in favour of resolved_paths since they both serve same purpose. Perhaps, we should deprecate Webpacker::Compiler.watched_paths.

cc// @javan

@gauravtiwari gauravtiwari requested a review from javan September 20, 2017 09:44
@maschwenk
Copy link
Contributor Author

Looking at it again, I just noticed that this also affects the dev environment as well

@gauravtiwari
Copy link
Member

Not sure I understand?

@maschwenk
Copy link
Contributor Author

maschwenk commented Sep 25, 2017

@gauravtiwari well the original commit for the default_watched_paths was mentioning lazy loading for test env, but I just noticed that the issue described at the top will affect both dev and test

@gauravtiwari
Copy link
Member

Ahh I see, yep it will do - it's intended to work for all environments 👍

@mgrsskls
Copy link

+1 for this. I added a folder to the resolved paths as well as the watched paths, but I was wondering why these files only get compiled when I restart my rails server. This PR seems to fix it, so it would be awesome if it could be merged!

@gauravtiwari gauravtiwari merged commit 9375c9e into rails:master Dec 15, 2017
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