-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adds compile option to configuration #503
Conversation
Instead of forcing default behavior of lazy-compilation in the test env, this change allows for lazy compilation to be configured via config/webpacker.yml by setting `compile` true/false. This would allow for expected behavior for the pack path lookup when using a webpack-dev-server process in the test environment.
+1 to making this configurable
Wouldn't running |
lib/install/config/webpacker.yml
Outdated
@@ -4,6 +4,7 @@ default: &default | |||
source_path: app/javascript | |||
source_entry_path: packs | |||
public_output_path: packs | |||
compile: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about flipping this around? Make it true
by default and false
in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since compilation only happens when packs are missing (vs. stale), the name compile
doesn't feel quite right. Perhaps something more descriptive like compile_missing_packs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Flipping the values and making the name more descriptive both sound like good changes.
lib/webpacker/configuration.rb
Outdated
@@ -36,6 +36,10 @@ def source | |||
fetch(:source_path) | |||
end | |||
|
|||
def compile? | |||
fetch(:compile).nil? ? Webpacker.env.test? : fetch(:compile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd drop the Webpacker.env.test?
default condition and just use the configuration option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern is for folks upgrading for whom the compile_missing_packs
entry would be missing... in this case, they would have to add in the value to get the expected behavior back in test
. Should we add a warning or upgrade note in README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rossta Yes please 👍 add it to Chagelog and highlight if it's a breaking change
@javan Thanks for the feedback.
You're right—it would've been more accurate for me to say the "lookup and compile if not found" behavior is only possible in |
abf96f9
to
4d58381
Compare
4d58381
to
b30a576
Compare
## Unreleased | ||
|
||
### Fixed | ||
- Update `webpack-dev-server.tt` to respect RAILS_ENV and NODE_ENV values [#502](https://github.com/rails/webpacker/issues/502) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this entry related to a change in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loosely. It was a necessary change along with this PR for the ability to run the webpack-dev-server in test. Would you prefer a separate PR for this entry?
Thank you! |
This change introduces a new configuration option for enabling lazy compilation via
config/webpacker.yml
.Along with #502, this would allow for running a webpack-dev-server process in the test environment instead of enforcing compilation to the
public
directory. The fallback for a missingcompile
key keeps the default lazy-compilation behavior intact.The
compile
key name borrows from theassets.compile
option for the asset pipeline.