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.

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

Perf: bail out fast if input doesn't contain stylex #44

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Dec 6, 2023

The PR applies performance improvements similar to johanholmerin/style9#74.

Currently, when the stylex babel plugin collects no stylex import, the babel plugin will bail out the transformation.

IMHO we can push that even further. The entire babel.transformAsync can be avoided for better performance if the input code doesn't contain the string stylex.

I've confirmed that after the changes, npm run test passed on my machine.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 6, 2023
@necolas
Copy link
Contributor

necolas commented Dec 6, 2023

FWIW we're planning to support configuration to process files using imports with names other than stylex, so at some point this probably wouldn't be a hardcoded string.

The last idea was had was to extend the importSources API (used to alias package name) to support defining custom specifiers too. For example

{
  importSources: [
    // name of package
    // `import * as styles from 'stylex'
    'stylex',
    // name of package and specifier
    // `import * as css from 'other-package'
    [ 'other-package', { specifier: 'css' } ]
  ]
}

Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea, but needs to account for inputSources

packages/nextjs-plugin/src/custom-webpack-loader.js Outdated Show resolved Hide resolved
@SukkaW
Copy link
Contributor Author

SukkaW commented Dec 6, 2023

@nmn @necolas Well, I found out that things are a little bit complicated:

  • The stylex's webpack plugin has already implemented fast bail-out:
    • if (
      this.stylexImports.some((importName) => inputCode.includes(importName))
      ) {
      const originalSource = this.babelConfig.babelrc
      ? await fs.readFile(filename, 'utf8')
      : inputCode;
      const { code, map, metadata } = await babel.transformAsync(
    • However, the bail-out was based on an option stylexImports, which is inconsistent as the Babel plugin's option name is importSources. The PR passes the webpack plugin's stylexImports to the Babel plugin's importSources.
    • Also, the default value of the webpack plugin's stylexImports is also wrong. The PR corrects that.
  • The stylex's Rollup plugin hasn't implemented the fast bail-out:
    • The PR adds that.

I have also rebased the PR to clean up the changes. The npm run test still passes on my machine after those changes.

@SukkaW SukkaW requested a review from nmn December 6, 2023 04:34
Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! Thanks for catching those mistakes.

@@ -84,6 +84,7 @@ class StylexPlugin {
type: 'commonJS',
rootDir,
},
importSources: stylexImports,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -23,6 +23,7 @@ module.exports = function stylexPlugin({
unstable_moduleResolution = { type: 'commonJS', rootDir: process.cwd() },
fileName = 'stylex.css',
babelConfig: { plugins = [], presets = [] } = {},
stylexImports = ['stylex', '@stylexjs/stylex'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably change this to just @stylexjs/stylex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we could keep ['stylex', '@stylexjs/stylex'] to prevent any potential breaking changes.

@@ -47,6 +48,11 @@ module.exports = function stylexPlugin({
return false;
},
async transform(inputCode, id) {
if (!stylexImports.some((importName) => inputCode.includes(importName))) {
// In rollup, returning null from any plugin phase means "no changes made".
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -48,7 +48,7 @@ class StylexPlugin {
dev = IS_DEV_ENV,
appendTo,
filename = appendTo == null ? 'stylex.css' : undefined,
stylexImports = ['stylex', '@stylex/css'],
stylexImports = ['stylex', '@stylexjs/stylex'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nmn
Copy link
Contributor

nmn commented Dec 6, 2023

Could you please run prettier so we can merge? Thanks!

@nmn nmn merged commit 7997652 into facebook:main Dec 6, 2023
9 checks passed
@nonzzz nonzzz mentioned this pull request Dec 6, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants