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

feat: exported return types of 'stylex.types.*' fns #867

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

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Jan 18, 2025

What changed / motivation ?

Fixes #857

New types are exported such as stylex.Color and stylex.Length.

These are the return types of the stylex.types.* function. So, for example,

  • stylex.types.color("red") is of type stylex.Color<string>

Question

Should the types be under stylex.types instead?

  • stylex.types.color("red") would be of type stylex.types.Color<string>

@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 Jan 18, 2025
@nmn nmn requested review from necolas, Jta26 and mellyeliu January 18, 2025 00:41
Copy link

github-actions bot commented Jan 18, 2025

workflow: benchmarks/size

Comparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.

@stylexjs/[email protected] size:compare
./size-compare.js /tmp/tmp.lCRZ6UqDYV /tmp/tmp.2mBN5zIAzV

Results Base Patch Ratio
stylex/lib/stylex.js
· compressed 729 1,013 1.39 !!
· minified 2,541 3,305 1.30 !!
stylex/lib/StyleXSheet.js
· compressed 1,266 1,266 1.00
· minified 3,776 3,776 1.00
rollup-example/.build/bundle.js
· compressed 567,110 567,110 1.00
· minified 10,232,512 10,232,512 1.00
rollup-example/.build/stylex.css
· compressed 100,628 100,628 1.00
· minified 755,718 755,718 1.00

@Jta26
Copy link
Contributor

Jta26 commented Jan 18, 2025

Should the types be under stylex.types instead?

I think this is a better organization in terms of usage with stylex.types functions, however it's kinda confusing when you have stylex.StaticStyles, which is also a "type". So the naming between TS/Flow types and CSS value types is kinda confusing.

This is also kinda awkward, but maybe valueTypes is better. This aligns with the actual name for such things.

https://www.w3.org/TR/css3-values/#component-types

@nmn
Copy link
Contributor Author

nmn commented Jan 19, 2025

This is also kinda awkward, but maybe valueTypes is better. This aligns with the actual name for such things.

I don't know if it's worth renaming the existing public APIs such as stylex.types.color() etc. And so it might make sense to keep that name.

stylex.types is about CSS types and not TS/Flow types such as stylex.StyleXStyles. Can we clarify this with documentation or is it too confusing and something that warrants an API change?

@Jta26
Copy link
Contributor

Jta26 commented Jan 20, 2025

Can we clarify this with documentation

Yeah documentation is probably the best way to disambiguate this.

@nmn nmn force-pushed the feat/export-type-types branch from 2e860bb to 72ab4b3 Compare January 22, 2025 03:41
@nmn
Copy link
Contributor Author

nmn commented Jan 22, 2025

There was no clean way to embed types under stylex.types that worked for both Flow and TS, so the types are now under stylex.Types.*.

This raises the question, is that the right name?

Should it be stylex.VarTypes.*? Or is stylex.Types better because it matches stylex.types?

@Jta26
Copy link
Contributor

Jta26 commented Jan 23, 2025

stylex.Types is next best imo.

Will the usage look like this?

xstyle: stylex.StaticStyles<{
 backgroundColor: stylex.Types.Color, (null | string)
 marginInlineStart: stylex.Types.Length, (null | string | number)
}>

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.

TypeScript error while using stylex.types - the inferred type cannot be named without a reference
3 participants