-
Notifications
You must be signed in to change notification settings - Fork 319
Add "extract" option to Rollup plugin #1604
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
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 36dba64 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ca6ac40
to
e967ff5
Compare
e967ff5
to
36dba64
Compare
display: 'block', | ||
...vars.typography.body.medium, | ||
|
||
'::before': { |
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 didn’t even look at these styles at all; they’re nonsense. But they’re just here to generate lines of code to test in the final bundle, and pull from the theme
{ | ||
"compilerOptions": { | ||
"baseUrl": "src", | ||
"module": "NodeNext", |
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.
NodeNext
is a minor thing to help the test more accurate:
- import * as styles from "./checkbox.css";
+ import * as styles from "./checkbox.css.js";
Requiring file extensions ensures we’re not making simple mistakes when matching modules. For example, it’d be easy to simply scan for /\.css$/
in imports, but that wouldn’t exist in other setups. People could alias it, rename it, etc. etc. So all that to say, by ensuring the imports don’t match the original filename, we’re leveraging Rollup properly to resolve and target the modules we want to bundle
* - custom function: takes an object parameter with `hash`, `filePath`, `debugId`, and `packageName`, and returns a customized identifier. | ||
* @default "short" | ||
* @example ({ hash }) => `prefix_${hash}` | ||
*/ |
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.
These JSDoc additions are all optional; can modify these if needed
* Name of emitted .css file. | ||
* @default "bundle.css" | ||
*/ | ||
name?: string; |
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 thought about allowing things like "[name]"
or "[hash]"
, but for reasons outlined in the PR notes, we’re not generating 1 per entry, so [name]
wouldn’t be respected. I didn’t know if anyone would need [hash]
. We could add it if needed.
return { | ||
name: 'vanilla-extract', | ||
|
||
buildStart() { | ||
extractedCssIds = new Set(); // refresh every build |
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 may be a little overly-defensive. I just wasn’t sure if there are scenarios where the root function scope sticks around between builds when in watch mode, etc.
@@ -0,0 +1,18 @@ | |||
{ | |||
"name": "@fixtures/react-library-example", |
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.
Added this fixture because for bundling I really wanted all the following conditions:
- both global and scoped CSS in the same package
- some deterministic ordering present (i.e. some CSS is expected to load before other)
- Vanilla Extract styles used in runtime, which didn’t have to be React, I was just too lazy to use another framework or do some pseudo-HTML template thing
// assert bundle CSS reflects order from @fixtures/react-library-example/index.ts | ||
const map = JSON.parse(String((sourcemapAsset as OutputAsset).source)); | ||
expect(map.sources).toEqual([ | ||
'src/styles/reset.css.ts.vanilla.css', |
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 probably the most important assertion out of the whole test, and gives great assurance it’s working as intended:
- The fact that it’s read off the
.css.map
file means it went into the actual.css
bundle, and isn’t just “floating around” in Rollup somewhere - The completeness of this list ensures all CSS made it into the bundle
- The order ensures it was bundled in module order, more-or-less (surprisingly, the order is not automatic—Rollup’s naive behavior is loading
utility.css
before all the React code)
// Emit .css assets | ||
moduleParsed(moduleInfo) { | ||
moduleInfo.importedIdResolutions.forEach((resolution) => { | ||
if (resolution.meta.css) { | ||
if (resolution.meta.css && !extract) { |
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 the only code point where previous behavior was modified. But since this is a new option, it’s backwards-compatible with all existing uses
// Generate bundle (if extracting) | ||
async buildEnd() { | ||
if (!extract) { | ||
return; |
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.
In the 2 new build stages, we’re only running code if extract
is specified, completing the loop on ensuring backwards-compatible behavior.
await buildAndMatchSnapshot({ | ||
format: 'esm', | ||
preserveModules: true, | ||
describe('Rollup settings', () => { |
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.
These tests were not modified at all! Choose Hide Whitespace to see a better view
} | ||
|
||
/** Compare import chains to determine a flat ordering for modules */ | ||
export function sortModules(modules: Record<string, ImportChain>): string[] { |
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.
Note: you’d really think that this would just come “for free” in Rollup, but it doesn’t. I know this seems like a big pile of indirection, and overkill, but it really is necessary for Rollup to bundle in the right order.
For example, take the fixture entry:
// 1. Style reset
import './styles/reset.css.js';
// 2. Design library
export { default as Button } from './button/button.js';
export { default as Checkbox } from './checkbox/checkbox.js';
export { default as Radio } from './radio/radio.js';
// 3. Utility CSS should be last
import './styles/utility.css.js';
If left to its own devices, Rollup bundles in this order, more-or-less:
- reset.css.ts
- utility.css.ts ← ❌ That’s a problem!
- vars.css.ts
- button.css.ts
- checkbox.css.ts
- radio.css.ts
This happens because it’s crawling from the entry, and the plugin stages fire basically as soon as it discovers a module. So in all the stages, you’re going in the order in which it was first seen, not necessarily where it sits in the final graph.
All the information about ordering is deep in Rollup’s graph, though! So all this function is doing is leveraging the information Rollup already has, to get the order we want. We’re not duplicating any work Rollup is doing; we’re only iterating over the module graph to sort the way we want.
Changes
Satisfies #1588. Adds an
extract
option to@vanilla-extract/rollup-plugin
that allows people to bundle one.css
file rather than leave imports intact. This is ideal for library builds that want to ship.css
directly, and not rely on downstream consumers to post-process the generated files.Alternatives
inject
mode where the JS injects<style>
tags (rollup-plugin-styles). I didn’t include that behavior here, but that could be added as a followup.css
directly, and is usually a workaround from some other limitation rather than an ideal approach. But if there’s a demand I don’t feel strongly enough to block folks from using it..css
imports inline and letting consumers take responsibility.import
statements at the end, we could have simply not generated them in the first place. But that means we’d have to assume responsibility for the mountain of work Rollup does for free: module graph scanning, treeshaking, parsing, and the list goes on.extract
option to fork code paths in the first step from the existing behavior, introduces risk as now there are almost 2 entirely-independent logic paths running alongside one another. It increases the possibility that users would get completely-different outputs based on a single setting, which isn’t ideal.Reviewing