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

Separate css extraction from build environment #1625

Merged
merged 2 commits into from Dec 14, 2018

Conversation

@tai2
Copy link
Contributor

tai2 commented Jul 26, 2018

webpacker currently tie CSS extraction to build environment. It extracts stylesheets to files when production build or development with no dev-server.
I want to separate it from build environment because CSS extraction is merely a user preference.
Some frontend developers might desire to put stylesheets in style tag or to emit separate css files using style-loader/url even in production.
These choices are reasonable in context of recent CSSinJS practices or HTTP/2 environment.

Another problem is that webpacker checks whether command line arguments contains a keyword webpack-dev-server to switch css extraction. It prevents to use other development tools like storybook, which contains their own webpack-dev-server, with webpacker's configuration file.
I bereave this PR simplifies the condition checking logic and provides more flexible configuration.

@tai2 tai2 force-pushed the tai2:extract-text-separation branch from afb4357 to 190d6a9 Aug 20, 2018
@gauravtiwari

This comment has been minimized.

Copy link
Member

gauravtiwari commented Dec 3, 2018

Thanks @tai2 Could you please rebase with master?

@tai2 tai2 force-pushed the tai2:extract-text-separation branch from 190d6a9 to f270950 Dec 13, 2018
@tai2 tai2 force-pushed the tai2:extract-text-separation branch from f270950 to 2f08533 Dec 13, 2018
@tai2

This comment has been minimized.

Copy link
Contributor Author

tai2 commented Dec 13, 2018

@gauravtiwari rebased 🙏🏻

@gauravtiwari gauravtiwari merged commit 41d79c9 into rails:master Dec 14, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gauravtiwari

This comment has been minimized.

Copy link
Member

gauravtiwari commented Dec 14, 2018

Thanks @tai2 🙏

@DougPuchalski

This comment has been minimized.

Copy link

DougPuchalski commented Jan 4, 2019

This is a breaking change, why default false?

@ericdfields

This comment has been minimized.

Copy link

ericdfields commented Jan 7, 2019

I get why it's now a config, but I did lose a bit of time wondering why my styles suddenly broke when a stray bundle install was done on a separate branch that got merged. Definitely strongly highlight this in the readme for upgraders!

@connorshea connorshea mentioned this pull request Jan 22, 2019
@zernie

This comment has been minimized.

Copy link
Contributor

zernie commented Jan 23, 2019

I had to spend a lot of time debugging this problem. Please consider enabling this by default on all environments.

@DougPuchalski

This comment has been minimized.

Copy link

DougPuchalski commented Jan 23, 2019

Every project needs to consider how much time a change, multiplied by the number of people it affects, can be extremely costly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.