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

Bump @types/react to ^17.0.42 #31894

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Bump @types/react to ^17.0.42 #31894

merged 1 commit into from
Mar 23, 2022

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Mar 20, 2022

WhiteSource Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
@types/react ^17.0.40 -> ^17.0.42 age adoption passing confidence

Configuration

πŸ“… Schedule: "on sunday before 6:00am" in timezone UTC.

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

β™» Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

πŸ”• Ignore: Close this PR and you won't be reminded about these updates again.


  • If you want to rebase/retry this PR, click this checkbox.

This PR has been generated by WhiteSource Renovate. View repository job log here.

@renovate renovate bot added the dependencies Update of dependencies label Mar 20, 2022
@mui-bot
Copy link

mui-bot commented Mar 20, 2022

No bundle size changes

Generated by 🚫 dangerJS against 5577f39

@mnajdova
Copy link
Member

@eps1lon I saw that you worked on DefinitelyTyped/DefinitelyTyped#58936 Can you share a bit of info about what should be the migration story here?

@eps1lon
Copy link
Member

eps1lon commented Mar 22, 2022

@eps1lon I saw that you worked on DefinitelyTyped/DefinitelyTyped#58936 Can you share a bit of info about what should be the migration story here?

Handle #31845 first and then see what's left to do.

@mnajdova
Copy link
Member

There are many issues, I have started creating a batch of the changes that we will need and try to test them on master first. @eps1lon this looks very much like breaking change to me, how was it released in a patch version? I know the major version needs to be compatible with the react's version but still looks like this change was not safe to be made as a patch.

@eps1lon
Copy link
Member

eps1lon commented Mar 22, 2022

this looks very much like breaking change to me, how was it released in a patch version?

Patches are technically never backwards compatible since people may rely on bugs. This is often the case with type patches. So yes, the @types/react change is a breaking change in many instances but it's intentional in many of them (e.g. ref={(element: HTMLElement) => element.focus()} would previously type-check but is clearly unsound since element is actually nullable at runtime) .

There can still be instances were this isn't intended so we need to investigate each type error that is now reported. If you find instances that are unclear or should not be rejected by TypeScript, then please point them out.

But a generic "type changes should not break tsc" is not helpful.

@michaldudak
Copy link
Member

In our case, many problems are caused by refs not accepting the supertypes anymore.
For example, it's not possible to pass a ForwardedRef to a ref prop accepting Ref (example from docs\data\material\components\selects\UnstyledSelectSimple.tsx, that previously checked well and now is triggers an error.

@mnajdova
Copy link
Member

mnajdova commented Mar 22, 2022

Regarding the subset <=> superset change, this is an interesting issue - #31928 where there basically isn't HTMLHeaderElement, so we cannot fallback anymore to HTMLElement.

@eps1lon
Copy link
Member

eps1lon commented Mar 22, 2022

At first glance this looks like we were just a bit inaccurate because polymorphic components are really hard. And this sloppyness was fine before because ref callbacks were bivariant. But it is unsound.

I wouldn't be that concerned if this were just subtypes of HTMLElement because you usually just want a generic HTMLElement anyway. In those scenarios I'd just revert the @types/react change and say it's too disruptive to make this sound (which is the TypeScript way).

But we also allow ignoring nullable instances which is the bigger problem here and which is harder to let go off.

Though on the flipside by making the callback bivariant we at least normalize behavior with ref objects because right now you can totally do declare const ref: { current: HTMLDivElement }; <div ref={ref} />; ref.current.focus() which type-checks but is unsound.

So this was helpful since it's now clear to me that the bivariance hack existed to normalize behavior between ref objects and ref callbacks. The current ref callback behavior is correct but ref objects are still unsound so let's prefer the consistent unsoundness over inconsistent soundness.

@michaldudak
Copy link
Member

Thanks for a quick response, @eps1lon!

@mnajdova
Copy link
Member

Thanks for checking this up @eps1lon

@renovate renovate bot changed the title Bump @types/react to ^17.0.41 Bump @types/react to ^17.0.42 Mar 23, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

We can merge now πŸ‘

@mnajdova mnajdova merged commit 50cbd21 into master Mar 23, 2022
@mnajdova mnajdova deleted the renovate/react-17.x branch March 23, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants