Skip to content
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

fix: Exclude placeholder prop from Modal props #2727

Closed
wants to merge 3 commits into from

Conversation

mdmower-csnw
Copy link
Contributor

@mdmower-csnw mdmower-csnw commented Jan 22, 2024

Summary

The types generated for Modal props indicate that placeholder is available because @types/react prior to v18.2.43 has placeholder as an attribute in HTMLAttributes. Beginning with @types/react v18.2.43, the placeholder attribute has been removed from HTMLAttributes. As a result, projects that consume this package but are themselves on @types/react v18.2.43 or newer no longer recognize placeholder as an optional property; they see it as stemming from these Modal props rather than JSX.IntrinsicElements['div'], and that it is required, not optional.

Patch Modal to manually omit placeholder from available properties without updating packages, which would require a more significant lift.

Ref: DefinitelyTyped/DefinitelyTyped#67170

How To Test

  1. Add react-uswds to a TypeScript project that has a dependency on @types/react v18.2.48.
  2. Use the <Modal> component without defining the placeholder attribute.
  3. Attempt to build the project -- the TypeScript compiler will report an error that placeholder is required but not defined:
    [tsl] ERROR in /path/sanitized/MyComponent.tsx(95,10) 
       TS2741: Property 'placeholder' is missing in type '{ children: Element[]; ref: RefObject<ModalRef>; "aria-describedby": string; id: string; className: string; }' but required in type 'Pick<ModalProps, "children" | "id" | "onError" | "key" | "aria-label" | "className" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | "suppressHydrationWarning" | ... 249 more ... | "isInitiallyOpen">'.
    

Additional Notes

  1. This PR would be obsoleted by feat!: React 18 Upgrade #2714 since that PR also updates @types/react to v18.2.48. That PR requires a major version update though, whereas this fix is safe to apply as a patch update.
  2. Modal was verified to be the only component affected by this issue by grepping built lib/components for string "placeholder". The only other component where the placeholder attribute appears explicitly is FileInput and there it is supported (HTMLInputElement instead of HTMLDivElement).

The types generated for Modal props indicates that placeholder is
available because @react/types prior to v18.2.43 has placeholder as an
attribute in HTMLAttributes. Beginning with @react/types v18.2.43, the
placeholder attribute has been removed from HTMLAttributes. As a result,
projects that consume this package but are themselves on @react/types
v18.2.43 or newer no longer recognize placeholder as an optional
property; they see it as stemming from these Modal props rather than
JSX.IntrinsicElements['div'], and that it is required, not optional.

Patch Modal to manually omit placeholder from available properties
without updating packages, which would require a more significant lift.

Ref: DefinitelyTyped/DefinitelyTyped#67170
@mdmower-csnw mdmower-csnw requested a review from a team as a code owner January 22, 2024 20:40
@werdnanoslen
Copy link
Contributor

Hey @mdmower-csnw thanks for this PR! We decided to proceed with #2714 — is there anything in this PR that you'd still like to submit, or should we close this?

@mdmower-csnw
Copy link
Contributor Author

@werdnanoslen - Thanks, that sounds fine. Feel free to close this.

It's worth keeping this PR in mind if an issue report comes in from a user on v5 or v6 who is not ready to jump to v7. They can either stay on @types/react@18.2.42 or this project could release patch updates to previous major releases (e.g. apply this commit on top of https://github.com/trussworks/react-uswds/tree/5.5.0 and release v5.5.1).

@werdnanoslen
Copy link
Contributor

or this project could release patch updates to previous major releases (e.g. apply this commit on top of https://github.com/trussworks/react-uswds/tree/5.5.0 and release v5.5.1).

Fair point! If anyone requires such a patch—and maybe comments here—we could re-open this PR to apply as you suggested. I'll close for now though.

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.

2 participants