Skip to content

fix: prevent style={undefined} from destroying computed styles in non-[style] targets#291

Merged
danstepanov merged 1 commit intonativewind:mainfrom
YevheniiKotyrlo:fix/deep-merge-config-undefined-styles
Mar 26, 2026
Merged

fix: prevent style={undefined} from destroying computed styles in non-[style] targets#291
danstepanov merged 1 commit intonativewind:mainfrom
YevheniiKotyrlo:fix/deep-merge-config-undefined-styles

Conversation

@YevheniiKotyrlo
Copy link
Copy Markdown
Contributor

@YevheniiKotyrlo YevheniiKotyrlo commented Mar 4, 2026

Summary

Fixes remaining Object.assign calls in deepMergeConfig that allow style={undefined} to destroy computed NativeWind styles on non-["style"] targets.

Problem

PR #224 fixed the most common code path (config.target = ["style"]), but two Object.assign({}, left, right) calls remain unfixed at lines 362 and 365.

Object.assign copies all enumerable own properties from right, including those with value undefined. When a component passes contentContainerStyle={undefined} alongside contentContainerClassName, the computed styles from left are overwritten with undefined.

Affected components:

  • ScrollViewcontentContainerClassName + contentContainerStyle={undefined}
  • FlatListcontentContainerClassName / columnWrapperClassName + *Style={undefined}
  • VirtualizedListcontentContainerClassName + contentContainerStyle={undefined}
  • Any custom styled() with non-default target mappings (e.g. { className: { target: "style" } })

This is a common pattern in reusable components where style props are optional:

function MyScrollView({ contentContainerStyle, ...props }: ScrollViewProps) {
  return (
    <ScrollView
      contentContainerClassName="p-4"
      contentContainerStyle={contentContainerStyle}
      {...props}
    />
  );
}
// <MyScrollView /> — contentContainerStyle is implicitly undefined, className styles lost

Native-only — web uses array concatenation (no Object.assign).

Solution

Added a mergeDefinedProps helper that replaces both Object.assign({}, left, right) calls. It spreads left into a new object and iterates right via for-in, skipping keys with undefined values.

function mergeDefinedProps(
  left: Record<string, any> | undefined,
  right: Record<string, any>,
) {
  const result = left ? { ...left } : {};
  for (const key in right) {
    if (right[key] !== undefined) {
      result[key] = right[key];
    }
  }
  return result;
}

This covers all config.target types (array, string, false) at all recursion depths, and still allows explicit null values to override (only undefined is skipped). Uses for-in instead of Object.keys() to avoid intermediate array allocation. Both left and right are always plain object literals (JSX props or calculateProps output) — no prototype chain concerns.

Performance

Benchmarked 15 implementation variants of mergeDefinedProps in isolation (2M iterations, best of 5 runs) across 6 scenarios matching real component renders, then validated the winner in the full deepMergeConfig context (1M iterations each):

Scenario Original Fixed Ratio
ScrollView + style={undefined} (the bug) 129ms 113ms 0.87x (13% faster)
ScrollView + valid style 125ms 122ms 0.97x (same)
FlatList + many props + undefined styles 320ms 302ms 0.94x (6% faster)
Custom styled() + style={undefined} 172ms 161ms 0.94x (6% faster)

The fix is faster than the original Object.assign across all real-world scenarios.

Verification

  • yarn typecheck — pass
  • yarn lint — pass
  • yarn build — pass (no unstaged files)
  • yarn test — 984 passed, 3 failed (pre-existing babel plugin failures, same on main)

Added 8 test cases to className-with-style.test.tsx covering all config.target types:

  • Path A (["style"]): style={undefined} and style={null} preserve computed styles
  • Path B (["contentContainerStyle"]): ScrollView and FlatList with contentContainerStyle={undefined} preserve computed styles; valid contentContainerStyle merges correctly; omitted prop preserves styles
  • Path B (string target): custom styled() with { target: "style" } preserves styles with style={undefined}
  • Real-world: optional contentContainerStyle prop forwarding (implicitly undefined) preserves computed styles

Reproduction

Visual reproduction repo with before/after screenshots: nativewind-style-undefined-repro

Before (3.0.4 unpatched)After (patched)

Related

@YevheniiKotyrlo YevheniiKotyrlo force-pushed the fix/deep-merge-config-undefined-styles branch 3 times, most recently from 191f035 to 6ef69c1 Compare March 4, 2026 18:37
@danstepanov
Copy link
Copy Markdown
Member

@YevheniiKotyrlo can you rebase your branch with main?

@YevheniiKotyrlo YevheniiKotyrlo force-pushed the fix/deep-merge-config-undefined-styles branch from 6ef69c1 to 761ca80 Compare March 9, 2026 10:21
@YevheniiKotyrlo
Copy link
Copy Markdown
Contributor Author

Done — rebased on latest main (251cdfc). All quality gates pass (typecheck, lint, build, tests).

@YevheniiKotyrlo YevheniiKotyrlo force-pushed the fix/deep-merge-config-undefined-styles branch from 761ca80 to efb6353 Compare March 12, 2026 19:20
@YevheniiKotyrlo
Copy link
Copy Markdown
Contributor Author

Rebased on latest main (b0b35b1, includes merged #277). Single clean commit.

All quality gates pass (typecheck, lint, build with no unstaged files, test — 984 passed, 3 pre-existing babel failures same on main).

@YevheniiKotyrlo YevheniiKotyrlo force-pushed the fix/deep-merge-config-undefined-styles branch 3 times, most recently from 3125b2c to 02ab6c4 Compare March 19, 2026 21:57
@YevheniiKotyrlo YevheniiKotyrlo force-pushed the fix/deep-merge-config-undefined-styles branch 2 times, most recently from 0631766 to 96c4da7 Compare March 20, 2026 18:50
@danstepanov danstepanov moved this to Todo in Roadmap Mar 26, 2026
@danstepanov danstepanov merged commit d53b870 into nativewind:main Mar 26, 2026
6 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in Roadmap Mar 26, 2026
@danstepanov danstepanov moved this from Done to Ready for Release in Roadmap Mar 26, 2026
YevheniiKotyrlo added a commit to YevheniiKotyrlo/react-native-css that referenced this pull request Mar 27, 2026
…e"] targets

When a component passes an empty style object (e.g., contentContainerStyle={{}})
to a non-["style"] target, mergeDefinedProps copies the empty object over the
computed className styles, destroying them. This is the same class of bug as
style={undefined} (fixed in PR nativewind#291) but for empty objects.

The ["style"] target path (Path A) already handles this correctly via
filterCssVariables({}) returning undefined. This fix extends mergeDefinedProps
with an isEmptyPlainObject check to skip empty objects on paths B and C,
matching Path A's behavior.

Common real-world trigger: components with default empty object parameters
({ contentContainerStyle = {} }) or explicit style={{}} props.
YevheniiKotyrlo added a commit to YevheniiKotyrlo/react-native-css that referenced this pull request Mar 27, 2026
…e"] targets

When a component passes an empty style object (e.g., contentContainerStyle={{}})
to a non-["style"] target, mergeDefinedProps copies the empty object over the
computed className styles, destroying them. This is the same class of bug as
style={undefined} (fixed in PR nativewind#291) but for empty objects.

The ["style"] target path (Path A) already handles this correctly via
filterCssVariables({}) returning undefined. This fix extends mergeDefinedProps
with an isEmptyPlainObject check to skip empty objects on paths B and C,
matching Path A's behavior.

Common real-world trigger: components with default empty object parameters
({ contentContainerStyle = {} }) or explicit style={{}} props.
danstepanov pushed a commit to YevheniiKotyrlo/react-native-css that referenced this pull request Apr 8, 2026
…e"] targets

When a component passes an empty style object (e.g., contentContainerStyle={{}})
to a non-["style"] target, mergeDefinedProps copies the empty object over the
computed className styles, destroying them. This is the same class of bug as
style={undefined} (fixed in PR nativewind#291) but for empty objects.

The ["style"] target path (Path A) already handles this correctly via
filterCssVariables({}) returning undefined. This fix extends mergeDefinedProps
with an isEmptyPlainObject check to skip empty objects on paths B and C,
matching Path A's behavior.

Common real-world trigger: components with default empty object parameters
({ contentContainerStyle = {} }) or explicit style={{}} props.
danstepanov pushed a commit that referenced this pull request Apr 8, 2026
…e"] targets (#312)

When a component passes an empty style object (e.g., contentContainerStyle={{}})
to a non-["style"] target, mergeDefinedProps copies the empty object over the
computed className styles, destroying them. This is the same class of bug as
style={undefined} (fixed in PR #291) but for empty objects.

The ["style"] target path (Path A) already handles this correctly via
filterCssVariables({}) returning undefined. This fix extends mergeDefinedProps
with an isEmptyPlainObject check to skip empty objects on paths B and C,
matching Path A's behavior.

Common real-world trigger: components with default empty object parameters
({ contentContainerStyle = {} }) or explicit style={{}} props.
@danstepanov danstepanov moved this from Ready for Release to Done in Roadmap Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants