-
Notifications
You must be signed in to change notification settings - Fork 313
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
Conversation
FWIW we're planning to support configuration to process files using imports with names other than The last idea was had was to extend the {
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' } ]
]
} |
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 a good idea, but needs to account for inputSources
@nmn @necolas Well, I found out that things are a little bit complicated:
I have also rebased the PR to clean up the changes. The |
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 looks good! Thanks for catching those mistakes.
@@ -84,6 +84,7 @@ class StylexPlugin { | |||
type: 'commonJS', | |||
rootDir, | |||
}, | |||
importSources: stylexImports, |
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.
👍
@@ -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'], |
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.
We can probably change this to just @stylexjs/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.
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; |
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.
👍
@@ -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'], |
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.
👍
Could you please run prettier so we can merge? Thanks! |
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 stringstylex
.I've confirmed that after the changes,
npm run test
passed on my machine.