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

Added option to disable async loading of CSS #348

Closed
wants to merge 4 commits into from

Conversation

thomasbaldwin
Copy link

@thomasbaldwin thomasbaldwin commented Feb 12, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

The reason for this PR are for the scenarios in which you have multiple branded websites, all using the same components, but different styles based off the brand. Lets say we have a component that looks like this:

import React from 'react';
import PropTypes from 'prop-types';
import { A, B, C } from 'config/brands';

const brandStyles = {};
brandStyles[A] = require('./styles.a.scss'); // version A of the component styling
brandStyles[B] = require('./styles.b.scss'); // version B of the component styling
brandStyles[C] = require('./styles.c.scss'); // version C of the component styling

const BrandedComponent = ({ brand, ... }) => {
  const styles = brandStyles[brand] || brandStyles[A];

  return (
    <div className={styles.container}>
      ...
    </div>
  )
}

export default BrandedComponent;

Notice how there's an A, B, and C stylesheet. This is because using a different stylesheet, we can drastically change how the component looks in our application, essentially creating different branded versions of it. Depending on the brand you hit, the correct stylesheet will be applied.

The way I've compiled the stylesheets is so that there's a global.css, a.css (all the styles.a.scss files), b.css (all the styles.b.scss files), c.css (all the styles.c.scss files) ...etc files. Depending on the brand you hit, I attach the correct stylesheets in a link tag to the page source so the browser will download them.

If I had not disabled the async loading of styles, the other brand styles being required at the top of the component would have been asynchronously downloaded, sometimes causing the component to not look as I wanted it to.

This has been asked for before in issues, so I figured we can't be the only ones who needed a feature like this:

#204

With this PR, you can now disable mini css extract from asynchronously downloading other stylesheets, by providing it the disableAsync option.

new MiniCssExtractPlugin({ filename: "[name].css", disableAsync: true })

Breaking Changes

None

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Feb 12, 2019

CLA assistant check
All committers have signed the CLA.

@thomasbaldwin thomasbaldwin changed the title Added option to disableAsync loading of CSS Added option to disable async loading of CSS Feb 12, 2019
@alexander-akait
Copy link
Member

Why don't use const brandStyles = {}; if (something) { brandStyles[A] = require('./styles.a.scss'); }?

@thomasbaldwin
Copy link
Author

thomasbaldwin commented Feb 12, 2019

Hey @evilebottnawi, thanks for your reply! I assume you're talking about something like this:

import React from 'react';
import PropTypes from 'prop-types';
import { A, B, C } from 'config/brands';

const { brand } = process.env;

const brandStyles = {};
if (brand === A) brandStyles[A] = require('./styles.a.scss'); // version A of the component styling
if (brand === B) brandStyles[B] = require('./styles.b.scss'); // version B of the component styling
if (brand === C) brandStyles[C] = require('./styles.c.scss'); // version C of the component styling

const styles = brandStyles[brand];

const BrandedComponent = () => {
  return (
    <div className={styles.container}>
      ...
    </div>
  )
}

export default BrandedComponent;

The reason we can't do something like this is because this would require the application to be built for a specific brand at a time (notice the brand condition is coming from an environment variable). By passing in the brand to be served through props, this gives you the ability to serve all brands in the same build & server and the decision of which brand to load is made during runtime.

@alexander-akait
Copy link
Member

alexander-akait commented Feb 12, 2019

@thomasbaldwin you can use document.querySelector('#theme') instead process.env. You try to solve problem what should be solved on code side. Also disabled async increase you bundle size, it is really bad for you clients (why i need download style of theme which i never will be used?) and bad option for plugin.

Better solution extract each theme in own chunk (splitChunks) and load their based on value of ui interface (example tag select)

@thomasbaldwin
Copy link
Author

thomasbaldwin commented Feb 12, 2019

@evilebottnawi document.querySelector is not isomorphic. The bundle sizes are not any bigger because we have all the different branded CSS put into their own compiled stylesheets (i.e. brandA.css, brandB.css, brandC.css) Based off the brand you hit, we attach the correct stylesheet to the page (so only the styles that are being used are being downloaded). We were able to cut the CSS we're delivering for each brand in half following this approach (since we're only delivering what's necessary for the specific brand you hit)

The only problem is when you load a component, mini css extract would asynchronously download the other brands stylesheets that we did not attach to the page source. The disableAsync option is there to make it so it won't do that if you don't want it to

@alexander-akait
Copy link
Member

alexander-akait commented Feb 12, 2019

@thomasbaldwin

document.querySelector is not isomorphic

What do you mean?

The only problem is when you load a component, mini css extract would asynchronously download the other brands stylesheets that we did not attach to the page source. The disableAsync option is there to make it so it won't do that if you don't want it to

It is break logic for async concept, just don't use plugin and include css using raw-loader or include their style in style/link tag

@thomasbaldwin
Copy link
Author

@evilebottnawi what I mean by document.querySelector not being isomorphic is that I can't say document.querySelector on the server side of my application because there doesn't exist a document. Only on the client side can I use a command like that. Our application is universal and the decision of which brand to load needs to be made on the server side.

The reason I've chosen mini css extract is because it will write the CSS to files (which I don't believe raw-loader will do), and using the splitChunks optimization, I'm able to configure which files are generated and how.

This is perfect, however, the issue we've run into is that when we attach a specific brand styling to the page source (i.e. <link href="brandA.css" rel="stylesheet" />), mini css extract would see the other requires in my component and go fetch them, which is why I believe there is a need for something like this.

@alexander-akait
Copy link
Member

alexander-akait commented Feb 12, 2019

@thomasbaldwin Why don't use document && document.querySelector ? document.querySelector('#theme') : defaultTheme? I think it is normal use default theme when client don't choose preferred theme.

You can save theme in cookie and use document && document.querySelector ? document.querySelector('#theme') : cookie.get('theme') ? cookie.get('theme') : defaultTheme

I don't think we really need this option, because right way is solve this problem on code side. It can also create bad practices for other developers.

@thomasbaldwin
Copy link
Author

@evilebottnawi that’s an interesting idea, but unfortunately the decision of which brand to load is not made by the client.

It’s actually made by the CMS’ API. We give it the URL that’s been hit, and it tells us what brand the page is and what components should appear on that page. Essentially every page is completely dynamic and controlled by the CMS.

I really like mini css extracts ability to create multiple compiled CSS files, I just wish we had some control over it asynchronously downloading things

@alexander-akait
Copy link
Member

@thomasbaldwin

We give it the URL

You need js API for this on CMS side. All works with CMS features should be done using API of CMS.

I wait answer from @sokra , but i vote 👎 on this option, it is bad practice and some new developers can starts using this.

@dmarcs

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@dmarcs

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@dmarcs
Copy link

dmarcs commented Feb 12, 2019

@evilebottnawi - how do I compile the stylesheets and write them to files without mini css extract? I am planning on using split chunks to split the CSS up, but what will turn my sass into css and write it to a file besides mini css extract?

What I don’t need is mini css extract to go and download CSS just because it sees it was required. I would like to have control over that. This feature would solve a lot of my problems

@alexander-akait
Copy link
Member

@dmarcs

I am planning on using split chunks to split the CSS up, but what will turn my sass into css and write it to a file besides mini css extract?

Please read documentation how splitChunks works and try examples.

This feature would solve a lot of my problems

No, it is create new problems for you and your clients.

@dmarcs
Copy link

dmarcs commented Feb 12, 2019

@evilebottnawi - as @thomasbaldwin pointed out, there are times when you want to have a skinned version of component driven by different stylesheets. Using split chunks and mini css extract, we can easily create stylesheets that contain only what is necessary for certain skinned versions to run. The issue though is that mini css extract will go and download styles being required, even if the runtime logic in the render method doesn’t say to use that stylesheet. By having the ability to disable mini css extracts async downloading of stylesheets, this would give me full control over what styles are being loaded

@alexander-akait
Copy link
Member

@dmarcs Please provide minimum reproducible test repo where this option required and you can't solve problem without this option

D-Marc1
D-Marc1 previously approved these changes Feb 12, 2019
Copy link

@D-Marc1 D-Marc1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what I needed!

@alexander-akait
Copy link
Member

@D-Marc1 It can't be merged without arguments, please provide your use case

@D-Marc1
Copy link

D-Marc1 commented Feb 12, 2019

Very similar issue to @thomasbaldwin. I have components that are controlled by different stylesheets, and am creating variation specific stylesheets:

For example:

Component1
-variationA.css
-variationB.css
-variationC.css

Component2
-variationA.css
-variationB.css
-variationC.css

Component3
-variationA.css
-variationB.css
-variationC.css

Depending on the domain the user hits, I will render the same site just styled differently (with more or less components placed on different areas of the page). For example, say you hit variationA.com, you will be given the variationA.css stylesheet, which contains all the styles from the variationA.css files. Likewise, if you hit variationB.com, you will be given the variationB.css stylesheet, which contains all the styles from variationB.css files.

I have code split off the components, and I only attach the components necessary for the site to run. However, because my components require all the different variation stylesheets in the global space, the other stylesheets are being fetched which results in more CSS than I need and also affects the styling of the component.

I know many people that are doing similar architectures to this in which they serve multiple branded websites off a single platofrm / server, and the decision of which brand / stylesheet to serve is made during runtime.

Having the ability to disable the async loading of stylesheets should have been an option from the beginning in my opinion. Thanks @evilebottnawi

@alexander-akait
Copy link
Member

@D-Marc1 you should load styles based on domain in your url, async logic same for any assets, like js, css, markdown and etc.

@alexander-akait
Copy link
Member

It is not extract plugin, it is plugin for css. Extract is a part of this job.

@thomasbaldwin
Copy link
Author

@evilebottnawi - What if there is no domain difference between variations? For example, you could have variations1 being served here:

website.com/this-is-url7138901312
website.com/this-is-another-url3

and variations2 being served here:

website.com/this-a-url-2
website.com/this-is-another-url712612

Because there's not an entire pattern I can rely on, I have to rely on my API rather than the domain to tell me what variation the user is hitting

@alexander-akait
Copy link
Member

@thomasbaldwin You should use CMS/framework API to get theme value, as i said above. All what you want should be done on code side, and most likely this option will never be merged.

@alexander-akait
Copy link
Member

Just note: PR should contains tests

@thomasbaldwin
Copy link
Author

@evilebottnawi sounds good I will add tests. I will also make a minimum reproducible repo so you can see the issue I'm running into and why I needed this feature. Thanks for your help :)

@lsycxyj
Copy link

lsycxyj commented Mar 21, 2019

@evilebottnawi I don't know if I've misunderstood the purpose of this pull request. If it's used to disable extracting the css of all async chunks, it think it should be useful. Because I don't need to make extra an request to fetch the css file of its own async js chunk which it has been loaded! It should be something like the "fallback" option of extract-text-webpack-plugin and its behavior.

For example, let's say we have several pages will be loaded on demand:

// routes.js
const routes: [
  {
    module: () => import('pageA')
  }
];

// pageA.js
import './pageA.css';
export default {
  // something that pageA should do
};

And the output files should be like this:

vendor.js
initial-chunk.js
initial-chunk.css
pageA.js
// Note that there isn't a "pageA.css" file

I wonder why the "fallback" option was removed in this plugin.

@ScriptedAlchemy
Copy link
Contributor

So you want to disable dynamic import() loading the stylesheets subsequent stylesheets?

@thomasbaldwin
Copy link
Author

thomasbaldwin commented Jul 30, 2019

@ScriptedAlchemy - I want to disable mini css extract from fetching CSS that is even mentioned in normal "requires" in a component. Currently, if you list multiple CSS files using require in a JS file, and only attach one of those CSS files through a link tag, mini css extract will by default go and download the other css files on the client side. I'm assuming the way they do this is through the dynamic import syntax like you posted, in which my answer would be yes.

@thomasbaldwin thomasbaldwin force-pushed the patch-1 branch 4 times, most recently from 4bcbcd4 to 5c416ec Compare July 31, 2019 03:00
@lsycxyj
Copy link

lsycxyj commented Jul 31, 2019

@thomasbaldwin I changed much more than your code because my feature can disable by each either entry or async module, instead of disabling all async modules.
In addition, your test case doesn't seem to be an async import. Or maybe my purpose is completely different from yours.

@anilanar
Copy link

anilanar commented Aug 8, 2019

I'm not convinced. @thomasbaldwin

Your component somehow has access to current brand. So you know the brand during runtime.

You also know how to map a brand to which file. So you can easily do the following:

const getBrandedStyles = brand => import(
  /* webpackChunkName: "styles-[brand]" */
  `./styles.${brand}.css`
);

const App = ({ brand }) => {
  const [styles, setStyles] = useState(null);
  useEffect(() => {
    getBrandedStyles(brand).then(styles => setStyles(styles));
  }, [brand]);
  return (styles ? <Loading /> : <BrandedComponent styles={styles} />);
}

This will create a css file for every file matching styles.___.css pattern and your runtime will only download it based on brand value.

You can use ContextReplacementPlugin to do more advanced mapping between brand and css file name.

@thomasbaldwin
Copy link
Author

thomasbaldwin commented Sep 25, 2019

@anilanar how would this work with SSR since useEffect is only called on the client side and ideally I could link to the css in the page source. I’ll take a look at the plugin

@anilanar
Copy link

@thomasbaldwin You implement a hook useStyles(brand). useStyles has different impls based on environment. If the environment is node, you don’t use useEffect. It’d directly require the style (use babel-plugin for replacing dynamic imports with require etc.)

@thomasbaldwin
Copy link
Author

@anilinar - I’m assuming I will have to traverse through the component tree and call this hook on every component during SSR, keep track of the stylesheets they are trying to load, and attach the corresponding stylesheets to the page source. Is that a correct assumption?

@anilanar
Copy link

anilanar commented Sep 26, 2019

@thomasbaldwin If you are trying to do what other CSS-in-JS solutions are already doing with their SSR, I’d check how they handle things before reinventing the wheel.

E.g instead of iterating the component tree, you can use a provider of your own (context api of react) and the useStyles hook would use the context to request a style. This way, your provider can exactly know which styles are used during the rendering.

Rolling your own solution to this complex domain is not easy though.

@thomasbaldwin
Copy link
Author

thomasbaldwin commented Sep 26, 2019

@anilanar Do you have an example of a CSS-in-JS solution that will handle code splitting multiple CSS files per 1 JS file. I believe MiniCSSExtract will only let me code split 1 CSS file per JS file

@anilanar
Copy link

@thomasbaldwin CSS-in-JS is just Javascript files that represent styles with JS modules. So code splitting js and styles has no difference. You can handle your requirements you mention above e.g with styled-components and ThemeProvider: https://www.styled-components.com/docs/advanced

However going with css-in-js, it’s not as easy to apply styles to your whole DOM tree with just 1 css file. Because css-in-js is supposed to isolate styles at component level. That’s its power, and maybe in your case its weakness.

@clucaseh
Copy link

Can we merge this?

@anilanar
Copy link

@clucaseh Potentially yes, but I have two proposals/additions:

  1. Maybe disableAsync could be renamed. What this option does is it prevents auto injection of <link href> elements to document.head when an async chunk is loaded via require.ensure or async import, similar to how extracted CSS are handled for entry chunks.

  2. For entry CSS files, a webpack user either has to use HTML webpack plugin to auto generate <link> tags or generate a webpack manifest which would be consumed by the server that serves the actual page etc. For async chunks, when this option is enabled, css must be manually injected, but how? How does runtime code know the css file path for the current chunk? e.g:

    import('./a.js').then(_module => /* what to do here? */);
    

    One option is to solve it at loader level. mini-css-extract-plugin's loader would export the url of the css file:

    // a.js
    import url from 'a.css';
    import myCache from './my-css-cache';
    
    if (!myCache[url]) {
        document.head.appendChild(/* element for <link rel="stylesheet" href={url} /> */);
        myCache[url] = true;
    }
    

@lsycxyj
Copy link

lsycxyj commented Oct 23, 2019

@anilanar Do you have an example of a CSS-in-JS solution that will handle code splitting multiple CSS files per 1 JS file. I believe MiniCSSExtract will only let me code split 1 CSS file per JS file

Can this PR solve your problem?

@clucaseh
Copy link

clucaseh commented Oct 23, 2019

2. e option is to solve it at loader level. mini-css-extract-plugin's loader would export the url of the css file:
// a.js

That sounds doable. @thomasbaldwin do you want to work on this?

I currently don't use html webpack plugin. We use c# razor so we don't use html pages. We are loading our main.css file in our layout as we want it to load as fast as possible. Currently trying to find a way to generate the CSS file but not have it imported in the final bundle.

Only way I have found to do this is to run two scripts on our end. First time we generate both the CSS and JS. Clear out the generated JS.

Run script again passing null-loader for CSS/Sass so no CSS is generated in the final JS. Which, will work it will just increase our build time. So this option would be nice for us.

@rjgotten
Copy link

rjgotten commented Oct 23, 2019

@clucaseh
Currently trying to find a way to generate the CSS file but not have it imported in the final bundle.

Use dynamic imports but place a conditional guard around it that only passes when code is built in dev mode - so that you can get hot reloading triggered by the import loading the JS and otherwise the import is ignored for purposes of code execution.

It should still be picked up and register as a dependency, leading to a separate (async) chunk with the JS hot loader code in there - which will just go untouched; not even loaded, in production mode. And mini-css-extract will ofcourse still generate a css file.

Should a direct if ( process.env === "development" ) lead to dead code removal and the import itself also not being processed, then store the result of the process.env comparison in a variable first. I know for sure that this would work around it, because it's what I'm using. 😉

@thomasbaldwin
Copy link
Author

@anilanar @clucaseh - Hey all, that sounds good I'll work on this :)

@danieliser
Copy link

How can we remove the async modules created by this plugin entirely?

Our use case is not an app or a library, its a WP plugin. We want to be able to bundle our CSS from components without having a separate entry point for each CSS chunk.

We do this perfectly with this plugin and a definition of splitchunks.

But if each splitChunk has the enforce option set to true, which is required to split the chunks into separate files, then our JS bundle never initiates.

On comparison it now has a webpackJsonpCallback block at the top of the bundle and is referencing deferredModules.push(["./src/index.js","block-editor-styles","block-styles","vendors~block-editor"]).

The first is our main chunk which is output as block-editor.js so shouldn't be async loaded, and the other 3 are caused by mini-css-extract.

If this option allows disabling that output to our JS bundle, then I disagree with the premise it should be relabeled for only preventing output to a browser of a link tag. We don't use any of that as we bundle to package as a release and manually enqueue our JS/CSS assets via the WordPress existing dependency mechanisms.

So there is a need for the originally described solution, the slimmed down version doesn't actually correct the underlying issue.

Sure in most cases actually loading your css async is great, but when given the constraint we are working inside another app, that simply isn't sufficient. Paths to the css/scripts can easily change from site to site making it not viable at all.

Here is sample code we are working from: https://github.com/PopupMaker/Block-Playground/blob/develop/webpack.config.js

As is the current code above outputs multiple CSS bundles correctly, but also builds that extra webpackJsonpCallback functionality into our final JS build.

@alexander-akait
Copy link
Member

Close in favor #432, feature will be in near future (this week hope)

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.

None yet