-
Notifications
You must be signed in to change notification settings - Fork 307
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
fix: rollup watch mode shouldn't clean cache #365
Conversation
c48bc34
to
03d984b
Compare
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.
Waiting for a response before this can be merged.
packages/rollup-plugin/src/index.js
Outdated
if (resolved && !resolved.external) { | ||
// $FlowExpectedError[object-this-reference] | ||
const result = await this.load(resolved); | ||
if (result) { | ||
stylexRules[resolved.id] = (result.meta: $FlowFixMe).stylex; | ||
} | ||
} |
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.
Can you write some comments explaining what is going on here?
AFAIK, the intention is to ensure that the generated styles from .stylex.js
files are included in the collected styles. I think this is needed because after compilation, this import becomes unneccessary and Rollup strips it out.
A simpler solution might be to add the treeshakeCompensation
option to the Babel plugin which inserts a side-effect-y import which won't be stripped out.
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.
However,I tested the treeshakeCompensation
option, But it did not affect the results. After my test, I discovered that when I modify a file, only the modified file triggers Rollup. Consequently, Rollup re-calls the buildStart, transform, or other hooks. However, the current behavior records only the modified file, preventing styleRules from capturing all the necessary information. To address this limitation, I added the following logic: I load the information from the Rollup cache and re-insert it.
Why
!resovled.external
- external means
node_modules
or other need exclude from bundle.
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.
What is happening on this line: if (filterByImportSource(importSources, stmt.source.value)) {
I believe this only works because it happens to match files that end with .stylex
, however, if you're looking for those files, this is the wrong function to use for it.
I'm still trying to make sense of the rest of the logic. But my understanding is this:
- If a file is part of the bundle, but then removed from the bundle.
- Then a different file imports that file
- Then that dependency is unchanged but should be part of the bundle again.
- Your code here manually goes through imports of the newly added/edited file and ensures they're included
Right?
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.
Let me explain why need'filterByportSource',AFAIK only include '.stylex.ext' can be handle. Oh maybe i ignored the follow case like define Theme. I only consider define variable.
About the issue listed, This logic only work at watch mode, Normally rollup will automatically trace them but sometimes rollup smart algorithms can not handle them.
(Although i manually them but the transform result isn't be change.Only load the cached style and re-insert) So in my option those logic are just to ensure the missing style rule can be insert.
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.
Here is some comment that after my test. Like stylex.defineVariable
will generate
export const xx = {
"variable":"var(--xx-hash)"
}
and other files who import those variables. According Rollup Document I think the logic can work well.
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.
Thanks for the PR. I'll have to spend some time with this to make sense of what is going on.
Merged in 20c637b |
What changed / motivation ?
Add disable clean cache logic for rollup plugin in watch mode.
Linked PR/Issues
Fixes #363
Additional Context
In Rollup watch mode, when a file is modified, the Rollup instance is reloaded, and all hooks are rerun. Currently, we clean the CSS record at buildStart so that in watch mode, only the modified file is recorded. However, other files that contain StyleX logic may be missing from the record.
First load
After modify
Pre-flight checklist
Contribution Guidelines