Skip to content
This repository has been archived by the owner on Oct 13, 2022. It is now read-only.

Emit css in modern build only #273

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

saylerb
Copy link

@saylerb saylerb commented Sep 30, 2020

Currently the template rollup config emits CSS twice if you are running build with the --legacy flag. This is resulting in duplicate css getting served to legacy browsers like IE.

This change changes the config to only emit css on the first pass.

@benmccann benmccann requested a review from antony October 1, 2020 03:01
@antony
Copy link
Member

antony commented Oct 1, 2020

@saylerb is this the right fix? I don't fully understand what emitCss does as I have proven to myself in the last few days, but if you don't emit css, what happens to it? Does it just end up written to the browser as part of the javascript? Or does it literally leave out css?

The docs don't make it any clearer for me.

@Conduitry
Copy link
Member

I've just tested this, and yeah, for the legacy build, this makes the CSS appear inline in the JS, which isn't what we want either.

@saylerb
Copy link
Author

saylerb commented Oct 2, 2020

@antony I have to admit I don't fully understand the emitCSS. But It appeared to me with it set to true, additional css files were getting written to the client/legacy/ folder that matched that in the client/. I can do a bit more digging into it

@Conduitry I can take a second pass. I don't think I saw the additional inline css.

@saylerb
Copy link
Author

saylerb commented Oct 10, 2020

I looked into this briefly, I think there may be a bug inject_scripts in how it is resolving the paths. If the CSS handling is being re-written, should I hold off on looking more deeply into this until 0.29 comes out?

@benmccann
Copy link
Member

benmccann commented Oct 10, 2020

There are not going to be any major changes to the CSS handling, so if there are any bugs that are there now they would continue to be present

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants