fix: prevent style={undefined} from destroying computed styles in non-[style] targets#291
Open
YevheniiKotyrlo wants to merge 1 commit intonativewind:mainfrom
Conversation
56daaa5 to
191f035
Compare
191f035 to
6ef69c1
Compare
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.
Summary
Fixes remaining
Object.assigncalls indeepMergeConfigthat allowstyle={undefined}to destroy computed NativeWind styles on non-["style"]targets.Problem
PR #224 fixed the most common code path (
config.target = ["style"]), but twoObject.assign({}, left, right)calls remain unfixed at lines 362 and 365.Object.assigncopies all enumerable own properties fromright, including those with valueundefined. When a component passescontentContainerStyle={undefined}alongsidecontentContainerClassName, the computed styles fromleftare overwritten withundefined.Affected components:
ScrollView—contentContainerClassName+contentContainerStyle={undefined}FlatList—contentContainerClassName/columnWrapperClassName+*Style={undefined}VirtualizedList—contentContainerClassName+contentContainerStyle={undefined}styled()with non-default target mappings (e.g.{ className: { target: "style" } })This is a common pattern in reusable components where style props are optional:
Native-only — web uses array concatenation (no
Object.assign).Solution
Added a
mergeDefinedPropshelper that replaces bothObject.assign({}, left, right)calls. It spreadsleftinto a new object and iteratesrightviafor-in, skipping keys withundefinedvalues.This covers all
config.targettypes (array, string, false) at all recursion depths, and still allows explicitnullvalues to override (onlyundefinedis skipped). Usesfor-ininstead ofObject.keys()to avoid intermediate array allocation. Bothleftandrightare always plain object literals (JSX props orcalculatePropsoutput) — no prototype chain concerns.Performance
Benchmarked 15 implementation variants of
mergeDefinedPropsin isolation (2M iterations, best of 5 runs) across 6 scenarios matching real component renders, then validated the winner in the fulldeepMergeConfigcontext (1M iterations each):style={undefined}(the bug)styled()+style={undefined}The fix is faster than the original
Object.assignacross all real-world scenarios.Verification
yarn typecheck— passyarn lint— passyarn build— passyarn test— 977 passed, 3 failed (pre-existing babel plugin failures, same onmain)Added 8 test cases to
className-with-style.test.tsxcovering allconfig.targettypes:["style"]):style={undefined}andstyle={null}preserve computed styles["contentContainerStyle"]):ScrollViewandFlatListwithcontentContainerStyle={undefined}preserve computed styles; validcontentContainerStylemerges correctly; omitted prop preserves stylesstyled()with{ target: "style" }preserves styles withstyle={undefined}contentContainerStyleprop forwarding (implicitlyundefined) preserves computed stylesReproduction
Visual reproduction repo with before/after screenshots: nativewind-style-undefined-repro
Related
style={undefined}still breaking after Feat/deep merge config optimizations #224)