Fix/custom element nullish prop coercion#14466
Conversation
📝 WalkthroughWalkthroughThe changes fix nullish value handling for custom element properties in the DOM patching module. Custom elements now preserve null values instead of coercing them based on property type, while standard elements maintain their existing coercion behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-dom/__tests__/patchProps.spec.ts`:
- Around line 67-70: The test assertion expects the string 'null' but patchProp
-> patchDOMProp for custom elements assigns the primitive null to el.value, so
update the assertion in patchProps.spec.ts to expect primitive null
(expect(el.value).toBe(null)) and ensure other assertions (e.g.,
expect(el.getAttribute('value')) and expect(el.setterCalled)) remain unchanged;
locate the test call to patchProp(el, 'value', null, null) and replace the
string assertion accordingly.
| patchProp(el, 'value', null, null) | ||
| expect(el.value).toBe('') | ||
| expect(el.value).toBe('null') | ||
| expect(el.setterCalled).toBe(2) | ||
| expect(el.getAttribute('value')).toBe(null) |
There was a problem hiding this comment.
Test assertion uses string 'null' instead of primitive null.
Tracing the new code path: patchProp(el, 'value', null, null) calls patchDOMProp with value = null. The key === 'value' early-return block (lines 27–55 of props.ts) is skipped for custom elements, so the new CE branch runs: needRemove = true, value is not coerced, and el.value = null is called directly. The custom setter stores null in this._value, so the getter returns null — not 'null'.
The PR objectives also state: "existing 'value for custom elements' expectation changed to null" — i.e. the primitive, not the string.
🐛 Fix the assertion
- expect(el.value).toBe('null')
+ expect(el.value).toBe(null)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| patchProp(el, 'value', null, null) | |
| expect(el.value).toBe('') | |
| expect(el.value).toBe('null') | |
| expect(el.setterCalled).toBe(2) | |
| expect(el.getAttribute('value')).toBe(null) | |
| patchProp(el, 'value', null, null) | |
| expect(el.value).toBe(null) | |
| expect(el.setterCalled).toBe(2) | |
| expect(el.getAttribute('value')).toBe(null) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/runtime-dom/__tests__/patchProps.spec.ts` around lines 67 - 70, The
test assertion expects the string 'null' but patchProp -> patchDOMProp for
custom elements assigns the primitive null to el.value, so update the assertion
in patchProps.spec.ts to expect primitive null (expect(el.value).toBe(null)) and
ensure other assertions (e.g., expect(el.getAttribute('value')) and
expect(el.setterCalled)) remain unchanged; locate the test call to patchProp(el,
'value', null, null) and replace the string assertion accordingly.
|
Closing as duplicate of #14459 |
2 participants
Summary
Fixes #14209
Custom element properties can accept various types. When a nullish value (null/undefined) or an empty string is provided, Vue should not coerce it according to the property's existing type.
In the previous implementation of
patchDOMProp, the nullish and empty value coercion logic would check thetype of el[key]and coerce it—even for custom elements:null→0for number propertiesnull→""for string properties""→truefor boolean propertiesWhile this was suitable for native HTML elements, it was incorrect for custom elements that can intentionally accept
null/undefinedor have varying types.The fix avoids this type-based coercion for custom elements (tag.includes('-')), preserving the value while still removing the attribute if it's nullish.
Test Plan
Summary by CodeRabbit
Bug Fixes: Custom elements maintain null and nullish property values as-is, while standard HTML elements retain existing coercion behavior.
Summary by CodeRabbit