fix(react): skip undefined props in attachProps (v9) - #31349
Closed
ptmkenny wants to merge 1 commit into
Closed
Conversation
|
@ptmkenny is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
2 tasks
2 tasks
Member
|
Hey @ptmkenny! Sorry to do this twice, but I'm closing this one too and going with a different approach in #31361 Skipping the assignment for nullish values changes what The new one mirrors |
ShaneK
added a commit
that referenced
this pull request
Aug 17, 2026
Issue number: resolves #31344 --------- ## What is the current behavior? Currently, `attachProps` assigns every prop onto the element without checking whether it has a value. Assigning a nullish value to a reflected property stringifies it, so an unset optional prop lands as `id="undefined"` and every element rendered that way shares an id. Setting the prop back to `undefined` overwrites the attribute rather than removing it. The same goes for `title`, `slot`, `lang`, and `dir`. ## What is the new behavior? We now remove the attribute after assigning, when the value was nullish and the name is a native property. That is what `@lit/react` already does in its `setProperty`, so the hand-rolled wrappers and the generated components behave the same way. ## Does this introduce a breaking change? - [ ] Yes - [X] No In fact, this version of this fix was made explicitly to avoid introducing more breaking changes at this time. ## Other information This replaces #31349, which fixed the same issue by skipping the property assignment for nullish values rather than removing the attribute afterwards. That changes what `undefined` means for a prop with a declared default. Today `<IonModal backdropDismiss={maybeUndefined} />` writes `undefined` over the `@Prop()` default of `true` so the backdrop doesn't dismiss, but with the assignment skipped the default survives and it starts dismissing. Same for `animated`, `showBackdrop`, and `keyboardClose`. It would also leave the package inconsistent, because the generated components go through `@lit/react`, which does assign, so `IonInput` and `IonButton` would disagree about what `undefined` means. Mirroring `@lit/react` avoids both, and this PR needs no BREAKING.md entry because of it. Note that the repro in #31344 uses `IonToggle`, which is generated on v9 and already passes there. This covers the hand-rolled wrappers `@lit/react` doesn't touch: the overlays, the routing proxies, and `IonApp`/`IonIcon`/`IonNav`/`IonRouterOutlet`/the tabs components. A prop removed from the JSX entirely, rather than set to `undefined`, still isn't cleared, since `attachProps` only iterates the new props. That's pre-existing and I left it alone, because three call sites pass a filtered `newProps` against the full `prevProps` so the fix isn't as simple as it would appear. Co-authored-by: Patrick Kenny github@ptmkenny.com ## Current dev build: ``` 8.8.19-dev.11786832241.150538c7 ```
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
2 participants
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.
Issue number: resolves #31344
This is the v9 version of the PR #31345