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

fix: usage of styles in a loop and non-existent styles #527

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Apr 9, 2024

Found and fixed a bug in the optimisation step for optimising stylex.props() and stylex() calls within a for-of loop.

The custom evaluate logic would fail to bail out and throw a null pointer exception instead.

The fix was to check for that null value and bail out instead.


Also, internally there are some rare example of non-existent styles (styles.unknown) being used which can cause the Babel plugin to throw an error.

Added a check to bail out instead of throwing an error for such cases and added a couple of test cases to verify that the fix works for stylex(), stylex.props() and stylex.attrs().

@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 Apr 9, 2024
@nmn nmn requested review from necolas and Jta26 April 9, 2024 06:09
Copy link

github-actions bot commented Apr 9, 2024

compressed-size: runtime library

Size change: 0.00 kB
Total size: 2.52 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./packages/stylex/lib/stylex.js 1.04 (3.22) 0.00 (0.00) 0.0% (0.0%)
./packages/stylex/lib/StyleXSheet.js 1.48 (3.75) 0.00 (0.00) 0.0% (0.0%)

Copy link

github-actions bot commented Apr 9, 2024

compressed-size: e2e bundles

Size change: 0.00 kB
Total size: 1128.74 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./apps/rollup-example/.build/bundle.js 1005.32 (10185.35) 0.00 (0.00) 0.0% (0.0%)
./apps/rollup-example/.build/stylex.css 123.42 (774.34) 0.00 (0.00) 0.0% (0.0%)

import stylex from '@stylexjs/stylex';
function test(colors, obj) {
for (const color of colors) {
obj[color.key] = stylex(color.style);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there already a test for this against the stylex.props syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it. For now I was testing the specific code that caused a breakage.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, it's worth testing everything against the main syntax first. Tests against the legacy syntax are only relevant for Meta.

@nmn nmn changed the title fix: usage of styles in a loop fix: usage of styles in a loop and non-existent styles Apr 9, 2024
@nmn nmn merged commit 3841145 into main Apr 10, 2024
9 checks passed
@nmn nmn deleted the fix/for-of-loop branch April 10, 2024 23:12
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