types(jsx): add undefined to optional properties #12771
Conversation
unrevised6419
force-pushed
the
fix/runtime-dom-optional
branch
5 times, most recently
from
cf3b819 to
fa1e744
Compare
Size ReportBundles
Usages
|
@vue/compiler-ssr
@vue/compiler-core
@vue/reactivity
@vue/runtime-core
@vue/compiler-dom
@vue/runtime-dom
@vue/compiler-sfc
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
edison1105
added
ready to merge
edison1105
changed the title
|
LGTM |
unrevised6419
force-pushed
the
fix/runtime-dom-optional
branch
from
0dc9169 to
5372774
Compare
WalkthroughThe PR updates TypeScript JSX interfaces in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime-dom/src/jsx.ts (1)
1-8: Nitpick: Document theexactOptionalPropertyTypesrationale
Consider enhancing the file header with a brief note explaining that these type changes support TypeScript’s--exactOptionalPropertyTypescompiler option and reference issue #6068 for context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
packages/runtime-dom/src/jsx.ts(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-dom/src/jsx.ts (1)
packages/runtime-core/src/vnode.ts (1)
VNodeRef(86-92)
🔇 Additional comments (6)
packages/runtime-dom/src/jsx.ts (6)
49-266: ARIA Attributes: Explicitly includeundefined
All attributes inAriaAttributesnow correctly include| undefined, satisfying--exactOptionalPropertyTypesrequirements. This change aligns with React’s patterns and improves type safety.
277-349: Standard HTMLAttributes: Add| undefinedfor optional props
The coreHTMLAttributesinterface has been updated so thatinnerHTML,style, and all standard HTML props explicitly allowundefined. This ensures JSX consumers can passundefinedwithout type errors under strict optional property settings.
362-385: Anchor & Area HTML Attributes: Explicitundefined
BothAnchorHTMLAttributesandAreaHTMLAttributeshave had their optional props (href,rel, etc.) updated to include| undefined. This is consistent with the PR goal and does not affect runtime behavior.
541-584: InputHTMLAttributes: Include| undefinedacross input props
TheInputHTMLAttributesinterface now explicitly adds| undefinedto every optional field (accept,checked,placeholder, etc.). This ensures compatibility with stricter TS settings when passingundefinedto inputs.
835-862: SVGAttributes: Explicitly allowundefinedon SVG props
All SVG-specific props (color,height,width,role,tabindex, etc.) now include| undefined, matching HTML and ARIA interfaces. Good coverage for strict optional types.
1425-1430: ReservedProps: Add| undefinedto key/ref props
TheReservedPropstype (key,ref,ref_for,ref_key) now explicitly allowsundefined, aligning with the rest of the JSX prop types.
unrevised6419
force-pushed
the
fix/runtime-dom-optional
branch
from
e3d3d94 to
bf55e8a
Compare
|
Note: In TypeScript v5.9, the |
|
Hey, @jh-leong, this PR has been open for a while, is there any way we can we move it forward? 👀 This probably can be tested like this, but for test to work, |
edison1105
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/runtime-dom/src/jsx.ts (4)
268-276: style includes undefined already via StyleValue (minor redundancy)StyleValue already allows undefined; keeping “| undefined” is harmless but redundant. If you prefer minimal unions, you can drop it.
- style?: StyleValue | undefined + style?: StyleValueSame applies to SVGAttributes.style below.
Also applies to: 281-282
835-844: SVG: minor nit on style; crossOrigin union looks fine
- style?: StyleValue | undefined is redundant like HTML (optional).
- crossOrigin union matches common usage.
- style?: StyleValue | undefined + style?: StyleValueAlso applies to: 858-862
277-349: Optional: add a CI canary to lock behavior under exactOptionalPropertyTypesAdd a tiny TSX compile check with exactOptionalPropertyTypes: true that sets several attributes explicitly to undefined (ref, key, aria-*, href, input props). This prevents regressions.
I can draft a minimal sample + tsconfig for a CI tsc step.
Also applies to: 541-584
49-266: No accidentalany | undefinedorundefined | undefinedpatterns found
Refine the “missing | undefined” check to only target quoted ARIA props
Run, for example:rg -nP "^\s+'aria-[^']+'\?:\s+(?!.*\|\s*undefined\b)(?!any\b)" packages/runtime-dom/src/jsx.tsto catch any ARIA attributes that lack
| undefinedwithout pulling in the element-to-interface mappings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/runtime-dom/src/jsx.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/src/jsx.ts (1)
packages/runtime-core/src/vnode.ts (1)
VNodeRef(86-92)
🔇 Additional comments (8)
packages/runtime-dom/src/jsx.ts (8)
49-266: Correct application of exactOptionalPropertyTypes across ARIA attributesAdding “| undefined” to optional ARIA props looks complete and consistent (including mixed/Booleanish and literal unions). No functional concerns.
278-349: HTMLAttributes now permit explicit undefined (as intended)innerHTML, role, inputmode, hidden, inert, etc., correctly accept explicit undefined. Matches React DT patterns.
362-372: Anchor/Area/Button attribute unions look correcthref/referrerpolicy/target/value unions mirror platform expectations and allow explicit undefined. Good.
Also applies to: 374-385, 389-410
541-584: InputHTMLAttributes: coverage and edge cases look goodKey spots (capture, checked with Set support, indeterminate, enterKeyHint, type, numeric bounds) correctly include undefined. Good catch on Set for multi-checkbox v-model.
755-771: Textarea value allows null and undefined (good)This aligns with DOM behavior and exactOptionalPropertyTypes needs.
863-1120: Broad SVG attribute sweep appears thoroughLarge set of attributes now permit explicit undefined; enums and Numberish usage are consistent. No obvious gaps spotted.
1122-1298: IntrinsicElementAttributes unchanged except types—OKNo structural changes beyond underlying attribute types. Looks good.
1425-1430: ReservedProps now accept explicit undefined (important for TSX “condition ? ref : undefined”)This avoids friction under exactOptionalPropertyTypes. Good change.
|
Thanks a lot @jh-leong 🥳 🎉 🎁 |
3 participants
close #6068
Allow TypeScript projects that have
exactOptionalPropertyTypesoption enabled to be able to pass explicit props with a value ofundefinedhttps://www.typescriptlang.org/tsconfig/#exactOptionalPropertyTypes
Let's take for example React:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b30cd2bf71cdef25085038b4a556211c23c16489/types/react/index.d.ts#L3446-L3729
Checklist:
any | undefinedundefined | undefinedI advise reviewing one commit at a time.
Images
"exactOptionalPropertyTypes": false"exactOptionalPropertyTypes": true"exactOptionalPropertyTypes": trueand fixedsrc?: string | undefinedRelated:
Links:
Summary by CodeRabbit