Skip to content

fix: add optional property types #6872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 19, 2024

Conversation

alecmev
Copy link
Contributor

@alecmev alecmev commented Aug 13, 2024

Addresses #1890 (comment)

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Install any of react-spectrum packages that bring in @internationalized/date and @react-types/shared, and type-check the project with exactOptionalPropertyTypes: true.

hourCycle causes this:

│ node_modules/@internationalized/date/dist/types.d.ts:615:11 - error TS2430: Interface 'import(".../node_modules/@internationalized/date/dist/types").ResolvedDateTimeFormatOptions' incorrectly extends interface 'Intl.ResolvedDateTimeFormatOptions'.
│   Types of property 'hourCycle' are incompatible.
│     Type '"h11" | "h12" | "h23" | "h24" | undefined' is not assignable to type '"h11" | "h12" | "h23" | "h24"'.
│       Type 'undefined' is not assignable to type '"h11" | "h12" | "h23" | "h24"'.
│ 
│ 615 interface ResolvedDateTimeFormatOptions extends Intl.ResolvedDateTimeFormatOptions {
│               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


│ node_modules/@internationalized/date/dist/types.d.ts:633:5 - error TS2416: Property 'resolvedOptions' in type 'DateFormatter' is not assignable to the same property in base type 'DateTimeFormat'.
│   Type '() => import(".../node_modules/@internationalized/date/dist/types").ResolvedDateTimeFormatOptions' is not assignable to type '() => Intl.ResolvedDateTimeFormatOptions'.
│     Type 'import(".../node_modules/@internationalized/date/dist/types").ResolvedDateTimeFormatOptions' is not assignable to type 'Intl.ResolvedDateTimeFormatOptions' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
│ 
│ 633     resolvedOptions(): ResolvedDateTimeFormatOptions;
│         ~~~~~~~~~~~~~~~

DOMProps causes this:

│ node_modules/react-aria-components/dist/types.d.ts:1148:18 - error TS2430: Interface 'GroupProps' incorrectly extends interface 'DOMProps'.
│   Types of property 'id' are incompatible.
│     Type 'string | undefined' is not assignable to type 'string'.
│       Type 'undefined' is not assignable to type 'string'.
│ 
│ 1148 export interface GroupProps extends AriaLabelingProps, Omit<HTMLAttributes<HTMLElement>, 'children' | 'className' | 'style' | 'role' | 'slot'>, _DOMProps1, HoverProps, RenderProps<GroupRenderProps>, SlotProps {
│                       ~~~~~~~~~~

@alecmev alecmev closed this Aug 13, 2024
@alecmev alecmev reopened this Aug 13, 2024
@alecmev alecmev force-pushed the fix-optional-property-types branch from c04c9f6 to 30e77c7 Compare August 24, 2024 14:05
@@ -59,7 +59,7 @@ export interface DOMProps {
/**
* The element's unique identifier. See [MDN](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id).
*/
id?: string
id?: string | undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks correct, but is really going to need to exist in so many more places
https://github.com/microsoft/TypeScript/blob/a86b5e2b01075db5046521958a3e0b905b4ca667/tests/lib/react18/react18.d.ts#L1863

Any way we can use the type from the original set instead of creating our own? maybe extends Pick<HTMLElement, 'id'>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The focus of this PR is to just fix the public API. I don't see anything wrong with adding | undefined in more places, but that's #1890.

Updated to use extends 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I guess i'm worried about potential regressions, and I'm also not sure that this is really all there is to fix in the public API, seems like too small a change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, there's most likely more, I only submitted the changes necessary to unblock our particular cocktail of Spectrum 😛 I'll create more PRs in the future as I run into other issues.

Hopefully #1890 will be addressed soon, but until then, one option could be to add a quick smoke test, that just lists every single package as a dependency and runs tsc with skipLibCheck: false, strict: true and exactOptionalPropertyTypes: true? This will allow you to make sure that the public API is clean, without switching the whole monorepo to exactOptionalPropertyTypes: true.

@alecmev alecmev requested a review from snowystinger August 27, 2024 15:01
@alecmev alecmev force-pushed the fix-optional-property-types branch from 5d21ca7 to 0ad564b Compare August 29, 2024 12:43
snowystinger
snowystinger previously approved these changes Aug 30, 2024
@yihuiliao yihuiliao changed the title Fix optional property types fix: add optional property types Oct 17, 2024
@unional
Copy link
Contributor

unional commented Oct 18, 2024

I keep getting into this issue. To use the types internally, I have AdjustExactOptionalProps from type-plus to do the adjustment, but then it fails when calling functions from rac.

@snowystinger snowystinger added this pull request to the merge queue Nov 19, 2024
Merged via the queue into adobe:main with commit b3a4d6c Nov 19, 2024
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants