-
Notifications
You must be signed in to change notification settings - Fork 11
Framework: Extract styles into 2 separate css files for better distribution and debugging #627
Conversation
8fce4cf
to
aa515fa
Compare
5d1fec0
to
07140b7
Compare
Here's what I think we should do as a short-term solution:
A better solution might be split |
About romainberger/webpack-rtl-plugin#5 This is a general plugin and I don't believe we should minify by defaut assets other than the one this plugin is responsible for.
|
@Tug I think it should, it already does it for the RTL css. |
I've pushed a commit that makes everything work™ It does that by referencing directly to:
romainberger/webpack-rtl-plugin#5 and:
When you run:
|
Indeed it works! Congrats @yurynix |
534e404
to
593f25d
Compare
958b54c
to
08142a0
Compare
08142a0
to
16acdde
Compare
I had to build and publish the forked version of Anyway it works now I think we're ready for a review! |
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.
This looks great. I left some notes, but they were mostly questions or references that I noticed that we should now remove. I agree that this is a good solution, and am mainly trying to understand the parts that confused me or the limitations of this and the previous solution.
We manage JS and CSS separately as it is supposed to be
- Do we? We still use CSS modules, the
withStyles
HOC, and determine style names programmatically with JavaScript. - For my own understanding: why is this "as it is supposed to be"? It seems arbitrary, but provides an advantage in terms of code-splitting. Managing markup and JS separately is also "as it is supposed to be".
If we didn't support RTL, I don't think it would necessarily be an advantage, as we now have a global CSS file containing styles for every section of the site.
loaders: [ | ||
{ | ||
test: /\.jsx?$/, | ||
loaders: [ 'react-hot' ] |
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'm betting this is my lack of knowledge about webpack showing, but why use react-hot
in production instead of development?
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.
That's probably another error error here, thanks for noticing, I'm going to have to fix that and test again the different envs...
// Switches loaders to debug mode. This is required to make CSS hot reloading works correctly (see | ||
// http://bit.ly/1VTOHrK for more information). | ||
config.debug = true; | ||
|
||
// Use a more performant type of sourcemaps for our development env | ||
// For a comparison see: https://webpack.github.io/docs/configuration.html#devtool | ||
// Enables source maps |
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.
Minor, but is there a reason this comment changed? This is slightly misleading, as source maps are enabled 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.
I'm not sure why it has changed but yeah we should keep the previous one
@@ -62,19 +61,24 @@ function renderPage( props, localeData ) { | |||
const assets = JSON.parse( fs.readFileSync( path.join( 'public', bundlePath, 'assets.json' ) ) ); | |||
// `main` is an array of JS files after a hot update has been applied | |||
const bundleFileName = typeof assets.main === 'string' ? assets.main : assets.main[ 0 ]; | |||
let stylesFileName = ''; | |||
if ( Array.isArray( assets.main ) ) { | |||
stylesFileName = assets.main.filter( asset => ( isRtl ? /rtl\.css$/ : /[^rlt].css$/ ).test( asset ) ).shift(); |
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 haven't tested this, so apologies if this is just my poor regex understanding, but do we need to escape the .
in the alternative condition? Also, rlt
seems like a typo, but I think those characters in a pattern like [^this]
can be in any order - is that correct?
if ( window.localeData ) { | ||
i18n.setLocale( window.localeData ); | ||
} else if ( currentRouteSlug ) { | ||
switchLocale( currentRouteSlug ); |
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.
For my own understanding: how would this condition ever be reached?
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 agree it's a bit confusing and I'm also having a hard time figuring out why I added this. I think it was made to display a localized 404 page using the locale slug. Since we're generating 404 pages now I don't think this is needed anymore.
@@ -26,5 +26,5 @@ export function addCss( css, styles ) { | |||
} | |||
|
|||
export function insertCss( styles ) { | |||
return styles._insertCss(); | |||
return styles._insertCss && styles._insertCss(); |
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.
For my own understanding: why do we need this presence check now? If _insertCss
isn't present in production/development, it might be worth commenting.
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.
We need that check because _insertCss
is added to every css modules by the isomorphic-style-loader
I believe. In production we don't use that loader and we don't need to manually append css blocks to the page since we have a bundle so we're just ignoring this function.
"clean:all": "rm -rf node_modules && npm run clean", | ||
"clean:i18n-cache": "rm -f server/i18n-cache/data/*", | ||
"clean:static": "rm -rf public/static", | ||
"generate": "npm run build && npm run build:static", | ||
"generate:rtl": "BUILD_RTL=true npm run build && npm run build:static:rtl", |
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.
We still reference BUILD_RTL
in the client webpack config. We should probably remove those ternaries.
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.
Yep sorry, I didn't cleanup after rebasing
@@ -39,7 +38,7 @@ var config = { | |||
}, | |||
|
|||
postcss() { | |||
return process.env.BUILD_RTL ? [ autoprefixer, rtlcss ] : [ autoprefixer ]; |
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.
We should probably remove the other reference to BUILD_RTL
below in this file as well.
"hooks": "node hooks/manage", | ||
"lint": "eslint --max-warnings 0 app client server && sass-lint --config .sass-lint.yml --max-warnings 0 --verbose", | ||
"preinstall": "npm run-script clean", | ||
"prod:static": "NODE_ENV=production npm install && NODE_ENV=production npm run generate && NODE_ENV=production npm run generate:rtl", | ||
"prod:static": "NODE_ENV=production npm install && NODE_ENV=production npm run generate", | ||
"qs": "npm run build && node server/build/bundle", | ||
"qs:rtl": "BUILD_RTL=true npm run build && node server/build/bundle.rtl", |
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.
We should probably remove this script, as well as start:rtl
.
I added a commit to address my notes about lingering |
I mean on production. I like the css module approach for developement but I don't think having our css bundled as text in our js is a good approach in production. Browsers are optimized to render css even before the js starts executing and I think generally we should keep doing what browsers would expect. |
1a585f3
to
cf53c35
Compare
The code looks good. I noticed that the hash on the LTR bundle is different to the RTL bundle. Is there a reason for that? |
@scruffian yes there are different for 2 reasons: |
Everything except CSS hot reloading in development seems to be working properly. When editing CSS, the app reloads but no new CSS is written onto the page. |
Product 👍
|
@drewblaisdell @fabianapsimoes actually this is broken in master as well, probably due to our upgrade to webpack2. We will need to fix that in another PR. Merging. |
@Tug have you opened another PR already? Otherwise, we should create an issue so it doesn't fall through the cracks. |
Now I have #683 |
This PR extracts the generated CSS into 2 separate external styleshets for ltr and rtl languages.
This has a few advantages over the current solution:
Testing Instructions
git checkout update/external-css
npm i
npm run start:static
Reviews