-
-
Notifications
You must be signed in to change notification settings - Fork 385
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
Conversation
Why don't use |
Hey @evilebottnawi, thanks for your reply! I assume you're talking about something like this:
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. |
@thomasbaldwin you can use Better solution extract each theme in own chunk ( |
@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 |
What do you mean?
It is break logic for async concept, just don't use plugin and include css using |
@evilebottnawi what I mean by 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. |
@thomasbaldwin Why don't use You can save 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. |
@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 |
You need I wait answer from @sokra , but i vote 👎 on this option, it is bad practice and some new developers can starts using this. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@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 |
Please read documentation how
No, it is create new problems for you and your clients. |
@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 |
@dmarcs Please provide minimum reproducible test repo where this option required and you can't solve problem without this 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.
This is exactly what I needed!
@D-Marc1 It can't be merged without arguments, please provide your use case |
Very similar issue to @thomasbaldwin. I have components that are controlled by different stylesheets, and am creating variation specific stylesheets: For example: Component1 Component2 Component3 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 |
@D-Marc1 you should load styles based on |
It is not extract plugin, it is plugin for css. Extract is a part of this job. |
@evilebottnawi - What if there is no domain difference between variations? For example, you could have variations1 being served here: website.com/this-is-url7138901312 and variations2 being served here: website.com/this-a-url-2 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 |
@thomasbaldwin You should use CMS/framework API to get theme value, as i said above. All what you want should be done on |
Just note: PR should contains tests |
@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 :) |
@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 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:
I wonder why the "fallback" option was removed in this plugin. |
So you want to disable dynamic |
@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. |
4bcbcd4
to
5c416ec
Compare
@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. |
I'm not convinced. @thomasbaldwin Your component somehow has access to current You also know how to map a
This will create a css file for every file matching You can use |
@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 |
@thomasbaldwin You implement a hook |
@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? |
@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 Rolling your own solution to this complex domain is not easy though. |
@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 |
@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 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. |
Can we merge this? |
@clucaseh Potentially yes, but I have two proposals/additions:
|
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. |
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 |
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 The first is our main chunk which is output as 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 |
Close in favor #432, feature will be in near future (this week hope) |
This PR contains a:
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:
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.Breaking Changes
None
Additional Info