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.
|
| (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.
| /** | ||
| * An undefined prop must never be assigned to the element. Reflected | ||
| * properties such as `id`, `title` and `slot` stringify whatever they | ||
| * are given, so `node.id = undefined` leaves the element with the | ||
| * literal attribute `id="undefined"`. `render()` already omits | ||
| * undefined props for the same reason. | ||
| * | ||
| * A prop that had a value and no longer does is a removal, so clear | ||
| * it the way React clears a removed attribute: reset the property | ||
| * first, for props with no attribute to mirror, then drop the | ||
| * attribute that a reflected property left behind. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * An undefined prop must never be assigned to the element. Reflected | |
| * properties such as `id`, `title` and `slot` stringify whatever they | |
| * are given, so `node.id = undefined` leaves the element with the | |
| * literal attribute `id="undefined"`. `render()` already omits | |
| * undefined props for the same reason. | |
| * | |
| * A prop that had a value and no longer does is a removal, so clear | |
| * it the way React clears a removed attribute: reset the property | |
| * first, for props with no attribute to mirror, then drop the | |
| * attribute that a reflected property left behind. | |
| */ | |
| /** | |
| * Reflected properties such as `id`, `title` and `slot` stringify | |
| * whatever they are given, so `node.id = undefined` leaves the element | |
| * with the literal attribute `id="undefined"`. Never assign an | |
| * undefined value. | |
| * | |
| * A prop that had a value and no longer does is a removal: reset the | |
| * property, which covers props with no attribute to mirror, then drop | |
| * the attribute a reflected property left behind. | |
| */ |
Two claims in here don't quite hold. The render() filter drops undefined as a side effect of its string | boolean | number allowlist, and its own comment gives the reason: objects, functions and arrays get synced as properties instead. Those are dropped by the same branch, so the "same reason" cross-reference contradicts itself if you follow it.
And React clears an attribute with removeAttribute, it doesn't assign undefined to the property first. The property write here is for props with no mirroring attribute, which is worth keeping in the comment, just not as React parity.
| */ | ||
| 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.
| 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.
|
|
||
| expect(div.hasAttribute('id')).toEqual(false); | ||
| expect(div.hasAttribute('title')).toEqual(false); | ||
| expect((div as any).testprop).toEqual(undefined); |
There was a problem hiding this comment.
| expect((div as any).testprop).toEqual(undefined); | |
| expect('testprop' in div).toBe(false); |
Nit: oldProps defaults to {} here, so nothing gets assigned and testprop was never on the node. The assertion as written passes on main too, so it isn't guarding anything. Checking the property is absent does fail before the fix. The same line in the second test is fine, since testprop actually gets set first.
Issue number: resolves #31344
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.