-
Notifications
You must be signed in to change notification settings - Fork 10
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
Enhancement/issue 427 css bundling rollup #438
Enhancement/issue 427 css bundling rollup #438
Conversation
Ok, so trying out some things and by making the whole
and get this for the
BUT, then I get this error from Rollup
And then I notice that the
No amount of Googling has really helped so far though... 🤷 Will see what happens, but will likely want to make an issue to specifically tracking this if I can't come to a resolution by the end of the week, so I can keep working on other things and testing our work so far via some alpha testing. |
I figured out the issue, I think it is with this postcss code, in particular the postcss(postcssConfig.plugins)
.use(postcssImport())
.process(source, { from: filePath })
.async(); Hardcoded that just to a string and omitting it and now files are getting emitted correctly. 💥 that.emitFile({
type: 'asset',
fileName,
name: href,
source: '.some-css { }'
}); Just need to figure out a way around not using edit: it's specifically related to the nano plugin being async, so will need to figure out a couple more things, but at least now it is behaving as expected output wise. |
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.
Figured out the issue with the tests and was definitely due to async
behavior within buildStart
in rollup.config.js. It seems PostCSS will only support being run asynchronously so when mixed with sync code like the parser and rollup (this.emitFile
) we were hitting all kinds of nasty race conditions.
So a little clunky of a solution for now, but at least reliable. Certainly room for improvement we can do via another PR.
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.
Removed support for theme.css config since it's not really needed now since any can BYOC (Bring your own CSS) and they already own the HTML file anyway. win-win, so just updated and enabled some additional test cases. This is all good to go now. 💯
* init draft of CSS bundling with rollup and moved out of copy lifecycle * emit and create CSS files in one place * manually processing CSS with PostCSS and got cssnano minification working * dont emit already emitted CSS link assets * address PostCss from option warning * disable theme.css tests in getting started case * convert postcss to async * CSS filename hashing * css bunding and support for relative @imports * remove logging * refactoring * temp work around for PostCSS and Rollup * delete describe.only * restored file name content hashing * aysnc refactor around PostCSS * added test cases for style and link tags, removed theme.css config * refine style test cases
* init draft of CSS bundling with rollup and moved out of copy lifecycle * emit and create CSS files in one place * manually processing CSS with PostCSS and got cssnano minification working * dont emit already emitted CSS link assets * address PostCss from option warning * disable theme.css tests in getting started case * convert postcss to async * CSS filename hashing * css bunding and support for relative @imports * remove logging * refactoring * temp work around for PostCSS and Rollup * delete describe.only * restored file name content hashing * aysnc refactor around PostCSS * added test cases for style and link tags, removed theme.css config * refine style test cases
Related Issue
resolves #427
Summary of Changes
<link>
gets inferred via Rollup now).<style>
and<link>
tags in their templatesNotes
The following will be resolved by #426
turns out rollup plugin postcss seems to only be for CSS-in-JS (e.g.
import 'something.css';
)should recommend "regular" CSS, or can we inline / inject / extract them all maybe?
TODO
emitFiles
to work (only) when running test cases. No CSS files are getting output to public/ :/