[babel-plugin] add LTR/RTL config to legacy-e… #1083
Conversation
workflow: benchmarks/perfComparison of performance test results, measured in operations per second. Larger is better.
|
workflow: benchmarks/sizeComparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.
|
packages/@stylexjs/babel-plugin/src/shared/physical-rtl/generate-ltr.js
Outdated
Show resolved
Hide resolved
packages/@stylexjs/babel-plugin/src/shared/utils/default-options.js
Outdated
Show resolved
Hide resolved
| @@ -53,19 +53,19 @@ describe('Legacy-shorthand-expansion resolution', () => { | |||
| var _inject2 = _inject; | |||
| import stylex from 'stylex'; | |||
| _inject2(".x123j3cw{padding-top:5px}", 4000); | |||
| _inject2(".x1mpkggp{padding-right:5px}", 3000, ".x1mpkggp{padding-left:5px}"); | |||
| _inject2(".x1gabggj{padding-right:5px}", 3000, ".x1gabggj{padding-left:5px}"); | |||
There was a problem hiding this comment.
Hmm can we avoid having these hashes change?
There was a problem hiding this comment.
i'm actually not sure why these are changing and have to investigate...
There was a problem hiding this comment.
just dug into this a bit. in the stylex-create visitor, we build the hash using the property name after shorthand expansion (padding -> padding-top, padding-start), but before ltr/rtl polyfill (see below).
the hashes changed because i made a change to expand to padding-inline-start instead of padding-start, but the polyfill later changes it back to paddingLeft and paddingRight
| _inject2(".x1hm9lzh{margin-left:10px}", 3000, ".x1hm9lzh{margin-right:10px}"); | ||
| _inject2(".x1sa5p1d{margin-right:10px}", 3000, ".x1sa5p1d{margin-left:10px}"); |
There was a problem hiding this comment.
Is it expected that the classname hash is the same for 2 different rules? Is this just how we've always done it?
There was a problem hiding this comment.
also dug into this, I think it's because:
<div class = "x1hm9lzh ..." />
we serve two different stylesheets depending on if it's LTR/RTL, but want to avoid adding two classnames, so in LTR it gets the .x1hm9lzh {margin-left } rule and in RTL it gets the .x1hm9lzh {margin-right}
packages/@stylexjs/babel-plugin/src/shared/utils/default-options.js
Outdated
Show resolved
Hide resolved
enableLegacyDirectionPolyfill config to legacy-e…| const propertyToLTR: $ReadOnly<{ | ||
| [key: string]: ($ReadOnly<[string, string]>) => $ReadOnly<[string, string]>, | ||
| }> = { | ||
| 'margin-start': ([_key, val]: $ReadOnly<[string, string]>) => [ | ||
| 'margin-left', | ||
| val, | ||
| ], | ||
| // 'margin-inline-start': ([_key, val]) => ['margin-left', val], |
There was a problem hiding this comment.
these are previous "aliases" that didn't need to be explicitly included, because we converted instances of marginInlineStart to marginStart while expanding shorthands. since we're expanding to marginInlineStart now, we need to include this in the mapping.
now included in inlinePropertyToLTR and used when LTR/RTL fallback is on and style resolution is legacy-expand-shorthands. after we ship this with experimental stylesheets we can try to remove most of this logic altogether
| ['marginBottom', bottom], | ||
| ['marginStart', left], |
There was a problem hiding this comment.
this is where we previously expanded margin to marginStart anyway, since it all got translated to marginLeft/marginRight downstream
| @@ -53,19 +53,19 @@ describe('Legacy-shorthand-expansion resolution', () => { | |||
| var _inject2 = _inject; | |||
| import stylex from 'stylex'; | |||
| _inject2(".x123j3cw{padding-top:5px}", 4000); | |||
| _inject2(".x1mpkggp{padding-right:5px}", 3000, ".x1mpkggp{padding-left:5px}"); | |||
| _inject2(".x1gabggj{padding-inline-end:5px}", 3000); | |||
There was a problem hiding this comment.
Maybe we should turn the feature flag on for these tests, if they're basically to test that the way we current use StyleX internally is not changed?
Context
Let's remove the LTR/RTL polyfill (#752), as most browsers support logical styles now:
legacy-expand-shorthandsworks in a few steps:legacy-expand-shorthands.js: We split shorthands into respective longhands. There are no (common) shorthands left after this./physical-rtlwe convert these into ltr:marginLeftand rtl:marginRight.Follow ups:
In 1., we do some transforms (marginInlineStart-> to RN equivalentmarginStart, for legacy internal reasons) that we need to remove, since this would be exposed without being converted tomarginLeftandmarginRightmargin-startcss propertiesborder-*-*-radiusproperties, though it's on the edge of browser support. They fit the internal browser requirements for every app except WhatsApp.Testing
For these tests, we expect to see the LTR/RTL polyfill when
enableLegacyDirectionPolyfill: true. We expect to the unmodified logical styles whenenableLegacyDirectionPolyfill: false.Note: We want to migrate over to
application-order(and laterproperty-specificity), but it'll be a long process to test and ship@layer. In the meantime, let's remove the LTR/RTL polyfill as a way to test experimental stylesheets. We use thelegacy-expand-shorthandsstyle resolution overapplication-orderorproperty-specificityinternally for this reason:legacy-expand-shorthandsgets rid of the most common shorthands and expands them, since without@layeruse inprocessStylexRules, we run into ordering issues with shorthands