fix(native): a target:false config gets its own scratch space, not style - #435
Open
YevheniiKotyrlo wants to merge 1 commit into
Open
fix(native): a target:false config gets its own scratch space, not style#435YevheniiKotyrlo wants to merge 1 commit into
style#435YevheniiKotyrlo wants to merge 1 commit into
Conversation
…tyle`
A `target: false` config means "write no style prop, route these declarations
into real props instead". Its declarations still need somewhere to sit between
`calculateProps` and `nativeStyleMapping`, and that place was `normal.style` —
`calculateProps` resolves the write target as `rule.target || "style"`, turning
the `false` into the string, and `nativeStyleMapping` reads it back out of
`props["style"]` by the same hardcoded name.
The pair is deliberate. What is not is that `style` is also a real target another
config can own. A component carrying both a `target: "style"` config and a
`target: false` config puts both sets of declarations in one object and each
drains the other's: with both naming `color`, the style-target config's
declaration disappears entirely and the other config's prop receives it. Order
makes no difference. The same shared space also shipped a `style` prop on a
config that asked for none, carrying whatever no mapping entry drained — a
`transform` among it, visibly moving a component that had declared it wanted no
`style` at all.
The scratch space is now the config's own source key. A source is an
`Object.entries(mapping)` key, so two configs cannot share one. No new mechanism
is introduced: `calculateProps` already nests an array target, and
`getStyledProps` already strips every consumed source — a `target: false` config
is always consumed, because `config.source !== config.target` holds for it.
`rule.target` and `config.target` now differ for these configs, which is the
point: the rule's target names which bucket the declarations go in, the config's
names which real prop the config owns. `config.target` is still `false` at every
reader, and `calculate-props.ts` is the only reader of `rule.target`. Web already
resolves a boolean target to the source key, so this moves native toward it.
Nothing shipped is affected. `Button` is the only component with `target: false`,
it carries no second config to swap with, and its mapping is the identity
`{ color: "color" }` — so nesting under `style` and writing at the top level are
indistinguishable for it. That is why 1048 tests passed either way.
A declaration the mapping does not name is now dropped rather than leaked into a
`style` prop the config declared it did not want.
Six tests, each red first and each proven red again with the fix reverted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
A
target: falseconfig means "write no style prop; route these declarations into real props instead". Its declarations still need somewhere to sit betweencalculatePropsandnativeStyleMapping, and that place isnormal.style:The pair is deliberate — that is how the declarations travel. What is not deliberate is that
styleis also a real target another config can own. A component carrying both kinds of config puts both sets of declarations in one object, and each drains the other's.stylelabelColor{ color: orange }bluemain{}blueThe style-target config's declaration is gone — drained into the other config's prop. It is order-independent, and it bites whenever the two configs name the same property.
The same shared space also ships a
styleprop on a config that asked for none, carrying whatever no mapping entry drained:{ fontSize: 20 }, or atransformthat visibly moves a component whose config said it wanted nostyleat all.Why the suite is green
Buttonis the only shipped component withtarget: false, and it misses this twice over: it carries no second config to swap with, and its mapping is the identity{ color: "color" }, so nesting understyleand writing at the top level are indistinguishable for it. React Native's ownButtonthen ignores the straystyleprop. Every other shipped component is bare-string or{ target: "style" }.So this reaches user-defined mappings, which
StyledConfigurationexplicitly supports —runtime.types.tscarries a dedicatedT extends falsebranch fornativeStyleMapping.The fix
Give the config its own scratch space instead of sharing one:
A
sourceis anObject.entries(mapping)key, so two configs cannot share one. No new mechanism is introduced:calculatePropsalready nests an array target, andgetStyledPropsalready strips every consumed source — atarget: falseconfig is always consumed, becauseconfig.source !== config.targetholds for it.rule.targetandconfig.targetnow say different things for these configs, and that split is the point: the rule's target names which bucket the declarations go in, the config's target names which real prop the config owns.config.targetis stillfalseat every reader, andcalculate-props.ts:58is the only reader ofrule.target.This also moves native toward web, where
useCssElementalready resolves a boolean target to the source key.What changes for callers
A declaration the mapping does not name is now dropped rather than moved into
style. That is the intended meaning oftarget: false— four places already say such a config owns no style prop — but it is a real behaviour change and invisible in a two-line diff:main.d { transform: translateX(10px); color: orange }, mapping{ color: "labelColor" }{ style: { transform: [...] }, labelColor: orange }{ labelColor: orange }{ labelClassName: false }shorthand{ style: { color: orange } }{}There is a runtime escape hatch, and it does not typecheck. A dotted destination writes wherever it points, so
{ color: "style.color" }puts a declaration back intostyle— butnativeStyleMappingdestinations are constrained to the component's own prop paths, and none reaches insidestyle. A typed caller therefore has no way to keep a declaration the mapping does not name. Worth your judgement on whether that gap should be closed, or whether dropping is simply whattarget: falseshould mean.One collision the types do not exclude: a config whose
targetnames another config'ssource. It is unreachable rather than unrepresentable — that mapping already throws inupdateRulestoday, onmain, whenever the source prop is passed, which is the same condition a scratch needs to exist at all. Filed separately rather than fixed here.Your own spec already says this
src/__tests__/native/styled.test.ios.tsxcarries a skipped test,static styles w/ target none, asserting that atarget: falseconfig with{ color: "myColor" }over.text-blue-500 { color: blue; background-color: red; }renders exactly{ testID, children, myColor }— unmappedbackground-colordropped, and nostyleprop.It is skipped because it names the option
nativeStyleToProp, which predatesnativeStyleMapping. Renaming that one key and running it:main#00fvs#0000ff), andstyle: { backgroundColor: "#f00" }The leaked prop that test forbids is exactly what this fixes. I have not un-skipped it here — the remaining mismatch is a stale colour format from an unrelated change, and updating it is not this PR's scope — but it is your statement of what
target: falseshould do, and this makes it true.Tests
Six in
src/__tests__/native/components.test.tsx, each written red first and each proven red again with the two lines reverted:target: falseconfig does not take a style-target config's declarationstarget: falseconfig emits no style proptarget: falseconfigs each keep their own declarationsnativeStyleMappingis called fornormalandimportant, so the one changed read has two call sitesstyleVerification
yarn typecheckandyarn lintclean. Full suite, same command both sides:f70c402)Exactly
+6. The 3 failures are thesrc/__tests__/babel/suites, which fail identically on the unmodified base — a Windows environment issue in my checkout.