fix(react): skip undefined props in attachProps - #31345
Conversation
|
@ptmkenny is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
changed the title
| (node as any)[name] = newProps[name]; | ||
| const propType = typeof newProps[name]; | ||
| const value = newProps[name]; | ||
| if (value === undefined) { |
There was a problem hiding this comment.
Passing null hits this same path. On this branch { id: null, title: null, slot: null } comes out as id="null" title="null" slot="null", and going from 'x' to null leaves id="null" instead of clearing. Same mechanism, since typeof null is 'object' so it skips the setAttribute branch but the property write still stringifies.
Since this is the same bug and v8 is still the current major, I think it's worth covering here rather than leaving it for a follow-up. The catch is that the obvious value == null widening would be a regression: null is a real value for some props (ion-input declares value?: string | number | null) and setting the property to null is how a controlled IonInput gets cleared. So it needs to be narrowed to the natively reflected properties.
There was a problem hiding this comment.
Yes, it makes sense to handle null too. I've updated the PR.
| */ | ||
| if (oldProps[name] !== undefined) { | ||
| (node as any)[name] = undefined; | ||
| node.removeAttribute(camelToDashCase(name)); |
There was a problem hiding this comment.
There are two attribute names in play for a camelCase prop, and this only clears one of them. The render() path emits access-key="k" because it dash-cases the prop name, while the property write here reflects to accesskey="k", so both end up on the element. On removal camelToDashCase gives access-key, which React has already dropped anyway, and the reflected accesskey="undefined" stays behind. Same for tabIndex, which leaves a stale tabindex="0". Dash-cased props are fine, myProp clears correctly.
Not a regression, since the old unconditional property write produced the same residue more often. The NON_BOOLEAN_FALSE_ATTRIBUTES set on major-9.0 already tracks this mapping from the dash-cased side, so it'd be good to have the reflected side covered too while we're in here. Up to you if it's in scope.
There was a problem hiding this comment.
Ok, I tried to fix this as well.
| expect(Object.keys((div as any).__events)).toEqual(['ionClick']); | ||
| }); | ||
|
|
||
| it('should not write undefined props to a dom node', () => { |
There was a problem hiding this comment.
Could you add a component-level test alongside these? The bug is an interaction between the render() filter, which omits undefined so React emits no attribute, and componentDidUpdate, which then writes it back, and a direct attachProps call can't see that. On main a wrapper mounted with id={undefined} still comes out as <ion-toggle id="undefined">, and these two tests wouldn't catch that coming back at the wrapper level.
There's precedent in this directory: createInlineOverlayComponent.spec.tsx already drives a generated wrapper with @testing-library/react, and createComponent pulls nothing from @ionic/core so there's no mocking needed.
There was a problem hiding this comment.
I added another test. I'll also submit a version of this for v9 shortly.
Co-authored-by: Shane <shane@shanessite.net>
…Props.ts Co-authored-by: Shane <shane@shanessite.net>
2 participants
Issue number: resolves #31344
V9 version: #31349
What is the current behavior?
When a React component forwards an optional prop to an Ionic component and the caller
leaves it unset,
@ionic/reactwrites the string"undefined"into the corresponding DOMattribute.
renders:
There is no error and no warning. Consequences:
duplicate ids (invalid HTML), and
document.getElementById('undefined')resolves towhichever comes first.
idis not special. Any prop backed by a reflected DOM property behaves the same way:title={undefined}produces a tooltip that reads "undefined", andslot={undefined}places the element in a slot named
undefined, which moves it in the layout.undefinedis not cleared: the attribute isoverwritten with
"undefined"rather than removed.What is the new behavior?
Skip undefined values, and treat a prop that had a value and no longer does
as a removal, which is how React handles it.
Does this introduce a breaking change?
Other information
Prepared with Claude Opus. This seems to be @ionic/react specific, as
attachPropsis React-only.