Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: rollup watch mode shouldn't clean cache #365

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: rollup watch mode shouldn't clean cache #365

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a257f32
fix: rollup watch mode shouldn't clean cache
nonzzz Jan 19, 2024
03d984b
fix: rollup plugin should record deps in watch mode
nonzzz Jan 21, 2024
ae32f79
chore: remove unnecessary condit
nonzzz Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 33 additions & 7 deletions packages/rollup-plugin/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,17 @@ export type PluginOptions = $ReadOnly<{
...
}>;

function filterByImportSource(
importSources: Options['importSources'],
code: string,
) {
return importSources.some((importName) =>
typeof importName === 'string'
? code.includes(importName)
: code.includes(importName.from),
);
}

export default function stylexPlugin({
dev = IS_DEV_ENV,
unstable_moduleResolution = { type: 'commonJS', rootDir: process.cwd() },
Expand Down Expand Up @@ -68,13 +79,7 @@ export default function stylexPlugin({
return false;
},
async transform(inputCode, id): Promise<null | TransformResult> {
if (
!importSources.some((importName) =>
typeof importName === 'string'
? inputCode.includes(importName)
: inputCode.includes(importName.from),
)
) {
if (!filterByImportSource(importSources, inputCode)) {
// In rollup, returning null from any plugin phase means
// "no changes made".
return null;
Expand Down Expand Up @@ -107,6 +112,27 @@ export default function stylexPlugin({
return { code: inputCode };
}

// $FlowExpectedError[object-this-reference]
if (this.meta.watchMode) {
// $FlowExpectedError[object-this-reference]
const ast = this.parse(code);
for (const stmt of ast.body) {
if (stmt.type === 'ImportDeclaration') {
if (filterByImportSource(importSources, stmt.source.value)) {
// $FlowExpectedError[object-this-reference]
const resolved = await this.resolve(stmt.source.value, id);
if (resolved && !resolved.external) {
// $FlowExpectedError[object-this-reference]
const result = await this.load(resolved);
if (result) {
stylexRules[resolved.id] = (result.meta: $FlowFixMe).stylex;
}
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

}
}
}
}

if (
!dev &&
(metadata: $FlowFixMe).stylex != null &&
Expand Down