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: Remove runtime injection from runtime #163

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Perf: Remove runtime injection from runtime #163

merged 2 commits into from
Dec 14, 2023

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Dec 13, 2023

What changed / motivation ?

The inject function has been removed from the main export of @stylex/stylex. Now the inject function can only be used by manually importing @stylex/stylex/lib/stylex-inject.

The Babel plugin has similarly been updated to import the inject function from the new location when runtimeInjection is turned on.

Also, runtimeInjection now accepts a string or {from: string, as: string} object to be able to override the inject function module location.

Additional Context

image

Tests have been updated to reflect the changes. The updated snapshots will show what has changed.

NOTE: The Webpack plugin is currently inlining the stylex-inject module in the generated code in the snapshot test. Trying to configure Webpack to not do that is causing strange errors.

Pre-flight checklist

  • Performed a self-review of my code

@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 13, 2023
@nmn nmn requested a review from necolas December 13, 2023 11:06
Copy link

github-actions bot commented Dec 13, 2023

compressed-size: runtime library

Size change: -0.07 kB
Total size: 2.29 kB

Filename: gzip (minify) kB size kB change % change
./packages/stylex/lib/stylex.js 0.95 (3.31) -0.07 (-0.22) -7.0% (-6.3%) 🟢
View unchanged
Filename: gzip (minify) kB size kB change % change
./packages/stylex/lib/StyleXSheet.js 1.34 (3.41) 0.00 (0.00) 0.0% (0.0%)

Copy link

compressed-size: e2e bundles

Size change: 0.00 kB
Total size: 1122.94 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./apps/rollup-example/.build/bundle.js 998.63 (9939.85) 0.00 (0.00) 0.0% (0.0%)
./apps/rollup-example/.build/stylex.css 124.30 (874.43) 0.00 (0.00) 0.0% (0.0%)

@necolas
Copy link
Contributor

necolas commented Dec 14, 2023

We should probably be checking the size of the bundled runtime in the CI task. It hasn't been showing the contribution of inject or styleq, so the size savings aren't showing up in the results

@nmn
Copy link
Contributor Author

nmn commented Dec 14, 2023

We should probably be checking the size of the bundled runtime in the CI task

I am looking into that. The numbers above are by using browserify, terser and gzip. I expect using rollup might squeeze this down a bit more.

@necolas
Copy link
Contributor

necolas commented Dec 14, 2023

oh actually it looks like styleq is inlined in lib/stylex.js, so we're good. the CI workflow already minifies and gzips.

Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

The webpack snapshot issue is because externals doesn't work on generated code, only source code. I looked into a bunch of potential alternatives, but nothing seems to work or doesn't support ESM syntax. Just a shortcoming of webpack

@nmn
Copy link
Contributor Author

nmn commented Dec 14, 2023

2.29 kB

This is in the ballpark of the size I got in local testing, but the comparison is still wrong since this PR cut the size in half.

Will figure this out later.

@nmn nmn merged commit f5e9b5e into main Dec 14, 2023
9 checks passed
@nmn nmn deleted the split-inject branch December 14, 2023 22:01
@necolas
Copy link
Contributor

necolas commented Dec 15, 2023

It's just because we don't inline ./stylex/lib/StyleXSheet.js into the main export of the package - the size of that file is listed separately in the "unchanged" files table. If we had size-diffed a production rollup bundle, we'd have seen the 3K reduction from inject not being imported anymore.

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

3 participants