fix(runtime-dom): preserve nullish values for custom element properties#14459
Open
apoorvdarshan wants to merge 1 commit intovuejs:mainfrom
Open
fix(runtime-dom): preserve nullish values for custom element properties#14459apoorvdarshan wants to merge 1 commit intovuejs:mainfrom
apoorvdarshan wants to merge 1 commit intovuejs:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change preserves nullish values (null/undefined) when writing properties to custom elements in patchDOMProp; custom element properties are no longer coerced to default primitives (e.g., '' or 0) while standard elements retain prior coercion behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Patch as PatchDOMProp
participant Elm as DOMElement
participant CE as CustomElement
Note over Patch,Elm: PatchDOMProp called with (el, key, value)
Patch->>Elm: inspect tagName / isCustom (contains '-')
alt is custom element
Patch->>CE: set property directly (value can be null/undefined/'')
CE-->>Patch: property assigned (no coercion)
Patch->>Elm: if value == null then remove attribute else keep
else standard element
Patch->>Elm: coerce value based on prop type (bool/string/number)
Elm-->>Patch: property/attribute updated (coerced value)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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.
🧹 Nitpick comments (2)
packages/runtime-dom/__tests__/patchProps.spec.ts (1)
79-105: Good coverage of all three primitive types and the''case; minor nit on thegetAttributeassertion.The test correctly exercises null on
number,boolean, andstringprops as well as the''-on-boolean edge case.One minor observation: Line 91's
expect(el.getAttribute('myNumber')).toBe(null)doesn't actually validate thatremoveAttributehad any effect, becausemyNumberwas never reflected as an attribute to begin with — the assertion passes trivially before thepatchPropcall too. To make this test more meaningful, the attribute could be set first:📝 Strengthened getAttribute assertion
+ el.setAttribute('myNumber', '1') // setting null on a number prop should preserve null, not coerce to 0 patchProp(el, '.myNumber', null, null) expect(el.myNumber).toBe(null) expect(el.getAttribute('myNumber')).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 79 - 105, The getAttribute assertion for myNumber is vacuous because the attribute was never set — before calling patchProp(el, '.myNumber', null, null) set the attribute on el (e.g., el.setAttribute('myNumber', '1')) so the test verifies that patchProp removes or clears the attribute; update the setup in the TestElement/test case to setAttribute on el for 'myNumber' prior to invoking patchProp and then assert el.getAttribute('myNumber') is null afterwards.packages/runtime-dom/src/modules/props.ts (1)
59-78: LGTM — logic is correct; minor semantic drift inneedRemoveworth noting.The control-flow split is well-placed and consistent with the existing
!tag.includes('-')guard on line 31. For the CE path,valueis preserved as-is and the attribute is removed only whenvalue == null— which aligns with the PR objective.One minor nit: the unchanged comment on line 108–109 reads "do not warn if value is auto-coerced from nullish values", but in the CE branch no coercion takes place —
needRemoveis being reused to mean "this is an intentional nullish assignment" rather than "coercion happened". The behaviour is correct, but a small follow-up comment clarification would keep future readers from being confused.📝 Suggested comment clarification (line 108)
- // do not warn if value is auto-coerced from nullish values + // do not warn if value is auto-coerced from nullish values, + // or if a nullish value is intentionally preserved for custom elements🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-dom/src/modules/props.ts` around lines 59 - 78, Update the existing comment that mentions "do not warn if value is auto-coerced from nullish values" to clarify that in the custom element branch (the guard using tag.includes('-')) no coercion occurs and that the needRemove flag is being reused to indicate an intentional nullish assignment rather than a coercion; keep the logic and the tag.includes('-') branch untouched and only change the comment near the code that sets needRemove for custom elements so future readers understand the different semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/runtime-dom/__tests__/patchProps.spec.ts`:
- Around line 79-105: The getAttribute assertion for myNumber is vacuous because
the attribute was never set — before calling patchProp(el, '.myNumber', null,
null) set the attribute on el (e.g., el.setAttribute('myNumber', '1')) so the
test verifies that patchProp removes or clears the attribute; update the setup
in the TestElement/test case to setAttribute on el for 'myNumber' prior to
invoking patchProp and then assert el.getAttribute('myNumber') is null
afterwards.
In `@packages/runtime-dom/src/modules/props.ts`:
- Around line 59-78: Update the existing comment that mentions "do not warn if
value is auto-coerced from nullish values" to clarify that in the custom element
branch (the guard using tag.includes('-')) no coercion occurs and that the
needRemove flag is being reused to indicate an intentional nullish assignment
rather than a coercion; keep the logic and the tag.includes('-') branch
untouched and only change the comment near the code that sets needRemove for
custom elements so future readers understand the different semantics.
3 tasks
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
edison1105
added
🔨 p3-minor-bug
Member
|
/ecosystem-ci run |
Contributor
|
📝 Ran ecosystem CI: Open
|
apoorvdarshan
force-pushed
the
fix/custom-element-nullish-prop-coercion
branch
from
487a8df to
f682338
Compare
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 79-105: The test only asserts null handling but should also assert
undefined handling for full "nullish" coverage: in the 'nullish value handling
for custom element props' test (class TestElement and calls to patchProp), add
additional calls that set each prop to undefined (patchProp(el, '.myNumber',
null, undefined), patchProp(el, '.myBool', null, undefined), patchProp(el,
'.myString', null, undefined)) and assert that el.myNumber, el.myBool, and
el.myString are undefined (and that attributes are removed or remain
null/undefined as appropriate, e.g.,
expect(el.getAttribute('myNumber')).toBe(null) or equivalent) to mirror the
existing null assertions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/patchProps.spec.tspackages/runtime-dom/src/modules/props.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime-dom/src/modules/props.ts
Custom element properties may accept multiple types, so when a nullish value (null/undefined) or empty string is passed, Vue should not coerce it based on the existing property type. Previously, null would be coerced to 0 for number props, '' for string props, and '' would be coerced to true for boolean props on custom elements. close vuejs#14209
apoorvdarshan
force-pushed
the
fix/custom-element-nullish-prop-coercion
branch
from
f682338 to
e682c35
Compare
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
3 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.
Summary
Fixes #14209
Custom element properties may accept multiple types, so when a nullish value (
null/undefined) or empty string is passed, Vue should not coerce it based on the existing property type.Previously in
patchDOMProp, the nullish/empty value coercion logic (lines 58-71) would checktypeof el[key]and coerce accordingly — even for custom elements:null→0for number propertiesnull→''for string properties''→truefor boolean propertiesThis is correct for native HTML elements (which have fixed property types), but wrong for custom elements whose properties can intentionally accept
null/undefinedor change types.The fix skips this type-based coercion for custom elements (
tag.includes('-')) and preserves the value as-is, while still removing the attribute when the value is nullish.Test plan
nullinstead of coerced''Summary by CodeRabbit
Bug Fixes
Tests