facebook
Valid StyleXTypes TS types
What changed / motivation ?
TypeScript types were invalid, fixed StyleXTypes.d.ts (VarGroup and CSSType) and added a compiled VarTypes.d.ts with conditions added to CSSType so type constraints are preserved.
For example:
(T extends (any extends Angle<infer R> ? R : never) ? Angle<T> : never)
Which resolves to:
(T extends string | 0 ? Angle<T> : never)
Instead of:
Angle<T>
Linked PR/Issues
Fixes #969
Additional Context
Screenshots, Tests, Anything Else
Pre-flight checklist
- [x] I have read the contributing guidelines Contribution Guidelines
- [x] Performed a self-review of my code
Hi @AlexanderOMara!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
Looks like there's some permission issue with the perf and size jobs failing to post these messages:
workflow: benchmarks/perf
Comparison of performance test results, measured in operations per second. Larger is better.
[email protected] compare node ./compare.js /tmp/tmp.3jhbpT0Mnk /tmp/tmp.mIMIagqG2O
| Results | Base | Patch | Ratio | |
|---|---|---|---|---|
| babel-plugin: stylex.create | ||||
| · basic create | 529 | 537 | 1.02 | + |
| · complex create | 180 | 193 | 1.07 | !! |
| babel-plugin: stylex.createTheme | ||||
| · basic themes | 453 | 454 | 1.00 | + |
| · complex themes | 42 | 41 | 0.98 | - |
workflow: benchmarks/size
Comparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.
[email protected] compare node ./compare.js /tmp/tmp.ZdyhaQDQfh /tmp/tmp.1NV3anEYCl
| Results | Base | Patch | Ratio | |
|---|---|---|---|---|
| stylex/lib/stylex.js | ||||
| · compressed | 900 | 900 | 1.00 | |
| · minified | 2,943 | 2,943 | 1.00 | |
| stylex/lib/StyleXSheet.js | ||||
| · compressed | 1,266 | 1,266 | 1.00 | |
| · minified | 3,776 | 3,776 | 1.00 | |
| benchmarks/size/.build/bundle.js | ||||
| · compressed | 537,611 | 537,611 | 1.00 | |
| · minified | 7,435,904 | 7,435,904 | 1.00 | |
| benchmarks/size/.build/stylex.css | ||||
| · compressed | 100,509 | 100,509 | 1.00 | |
| · minified | 754,513 | 754,513 | 1.00 |
I think it would be even better if we could get the generated TS types to be correct, whether than involves fixes to flow-api-translator or changes to the source flow types so that it can be converted to valid TS. And have some way for CI to surface if the generated TS types become invalid, to catch any regressions in patches. That will be valuable because we'd like to rewrite the Flow types at some point - we don't use TS at Meta, and realistically won't be thinking to update them here, so adding more non-generated TS files is kind of going in the opposite direction.
I'm not sure what all is wrong with transpilation for StyleXTypes.d.ts that prompted the manual TS file, but for CSSType anyhow:
Fixes to flow-api-translator could be possible. Essentially it would need to look at the types it aliases, and for any that have more-narrow types, add conditions like I did in this PR. It's not as simple as rewritting the syntax, it would need to be aware of the types in the generics it aliases. Not knowing how the Flow->TS transpiler works, is it advanced enough for something like that?
Changes to the source Flow type I'm unclear on (as you can probably imagine), but it doesn't seem to have a syntax similar to the TypeScript syntax. I guess looser types could be used all around, but maybe not desirable.
EDIT: Actually, I have an idea about how to improve this, and maybe avoid the need to know what the generic type constraints are in the type alias.
Looks like the Flow->TS transpiler operates on the code of a single file, so it would not be able to explicitly add the type conditions in the general case.
I updated this PR with more-advanced TS code to dynamically extracts the generic constraints for CSSType. I think the Flow->TS transpiler could more-easily generate code like this which would make it more-accurate, although I'm not sure how hard it would be to implement.
Created as issue here: https://github.com/facebook/hermes/issues/1694
I took a crack at this in #1024 in a way where the existing generation of the Typescript types can be valid.
If you don't mind, please take a look at it @AlexanderOMara
Fixed in #1024, although there is still no automatic generation of the TS types.