Fix v11.0.0 types that removed the default export #1406
Conversation
Fixes #1396 My original intent was to remove the default export (a breaking change) as part of the major 11.0.0 release but I totally flopped on that by not also actually providing the named export in the code. Now that the major release ship has sailed, this change fixes the types by reinstating the default export in the typings but marking it deprecated, while also providing the new named export alternative as the way forward.
🦋 Changeset detectedLatest commit: 120f289 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This replaces #1397 |
There was a problem hiding this comment.
I've verified these changes to work as expected in a (private) app that uses focus-trap-react, both with the old default export, and with the new named export. The updated types also work.
However, I believe it would be an improvement to not extend React.AllHTMLAttributes, see comment.
index.d.ts
Outdated
| @@ -1,14 +1,56 @@ | |||
| import { Options as FocusTrapOptions } from 'focus-trap'; | |||
| import * as React from 'react'; | |||
|
|
|||
| export interface FocusTrapProps extends React.AllHTMLAttributes<any> { | |||
There was a problem hiding this comment.
Out of curiosity, why extend React.AllHTMLAttributes here? I know it was done in earlier releases as well, but as far as I can see, none of the normal HTML attributes get used for anything. For example if I use
<FocusTrap className="myFocusTrap">
<div></div>
</FocusTrap>Then there is no type error, but the class name "myFocusTrap" doesn't get applied to any element.
Wouldn't it be better to remove this altogether?
| export interface FocusTrapProps extends React.AllHTMLAttributes<any> { | |
| export interface FocusTrapProps { |
There was a problem hiding this comment.
It's just been like that "forever" (before my time leading this project) but now that you point this out, you're totally right: There's no point in having this extension because <FocusTrap> doesn't render to anything, and doesn't reflect those extra HTML props onto anything either.
And it should be safe to remove that because setting something like className would have never done anything anyway, so I would expect nearly no one is doing this today anyway.
There was a problem hiding this comment.
@Mathias-S PR updated!
|
@Mathias-S Thank you for your feedback! Merging and publishing. |
Fixes #1396
My original intent was to remove the default export (a breaking change) as part of the major 11.0.0 release but I totally flopped on that by not also actually providing the named export in the code.
Now that the major release ship has sailed, this change fixes the types by reinstating the default export in the typings but marking it deprecated, while also providing the new named export alternative as the way forward.
PR Checklist
Please leave this checklist in your PR.
typeof document/window !== 'undefined'before using it in code that gets executed on load.npm run changesetlocally to add one, and follow the prompts).