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.

Sign up for GitHub

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

Jump to bottom

[react] Make all refs mutable by default #64896

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

[react] Make all refs mutable by default #64896

wants to merge 13 commits into from
+94 −98

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 26, 2023 •
edited

Ready for review but won't be merged here and rather in a PR for 19 stacking all changes from #64451

Closes #64855
Closes #64772
Fixes first gotcha in https://twitter.com/mattpocockuk/status/1636098722982404096 (second one will be fixed in #64920).

 interface RefObject<T> {
-   readonly current: T | null
+   current: T
 }

RefObject is no longer considered a ref managed by React (i.e. it no longer makes current nullable). Instead, current will only have the type you declare and be mutable.

Historically RefObject was the type returned from createRef so it made sense to always include null since that what React does on initialization. The mistake we made was reusing it for useRef which has a different audience (useRef for function components where it can act as "instance variables", createRef for class components where we only care about React managed refs).

But now that we moved along it makes sense to fix this incorrect abstraction, take on a breaking change and reduce papercuts going forward.

Migration

  1. Update all @types/* packages to their latest versions
  2. Apply npx types-react-codemod refobject-defaults ./src (see types-react-codemod for full usage)

The remaining fixes should only include libraries shipping their own types. Either these libraries are already compatible with React 19 types and you just need to upgrade them or you should raise an issue on their repositories notifying them that their types are not compatible with React 19.

@typescript-bot typescript-bot added this to Needs Author Action in New Pull Request Status Board Mar 26, 2023
@eps1lon eps1lon mentioned this pull request Mar 26, 2023
Merged
2 tasks
@eps1lon eps1lon force-pushed the feat/react/new-refs branch 2 times, most recently from ffff287 to 95c7b2e Compare March 26, 2023 20:59
@DangerBotOSS
Copy link

DangerBotOSS commented Mar 26, 2023 •
edited

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

babel-plugin-react-html-attrs (unpkg)

was missing the following properties:

  1. The declaration doesn't match the JavaScript module 'babel-plugin-react-html-attrs'. Reason:
    The JavaScript module can be called or constructed, but the declaration module cannot.

gatsbyjs__reach-router (unpkg)

was missing the following properties:

  1. BaseContext
  2. LocationContext
  3. insertParams
  4. match
  5. navigate
as well as these 34 other properties...

pick, resolve, shallowCompare, startsWith, useBaseContext, useLocationContext, useNavigate, validateRedirect, BaseContext, LocationContext, insertParams, match, navigate, pick, resolve, shallowCompare, startsWith, useBaseContext, useLocationContext, useNavigate, validateRedirect, BaseContext, LocationContext, insertParams, match, navigate, pick, resolve, shallowCompare, startsWith, useBaseContext, useLocationContext, useNavigate, validateRedirect

reach__router (unpkg)

was missing the following properties:

  1. matchPath
  2. matchPath

react-bootstrap-typeahead (unpkg)

was missing the following properties:

  1. useHint
  2. Input
  3. asyncContainer
  4. useAsync
  5. withAsync
as well as these 5 other properties...

menuItemContainer, useItem, withItem, tokenContainer, withToken

react-map-gl (unpkg)

was missing the following properties:

  1. AttributionControl
  2. setRTLTextPlugin
  3. MapContext

react-onclickoutside (unpkg)

was missing the following properties:

  1. IGNORE_CLASS_NAME
  2. IGNORE_CLASS_NAME

react (unpkg)

was missing the following properties:

  1. unstable_act

tuya-panel-kit (unpkg)

was missing the following properties:

  1. Drawer
  2. Wave
  3. Diffusion
  4. OfflineView
  5. FullView

wordpress__components (unpkg)

was missing the following properties:

  1. CustomGradientPicker
  2. DuotonePicker
  3. DuotoneSwatch
  4. GradientPicker
  5. GuidePage
as well as these 17 other properties...

Line, SearchControl, TextHighlight, ToolbarDropdownMenu, ToolbarItem, useBaseControlProps, CustomGradientPicker, DuotonePicker, DuotoneSwatch, GradientPicker, GuidePage, Line, SearchControl, TextHighlight, ToolbarDropdownMenu, ToolbarItem, useBaseControlProps

wordpress__rich-text (unpkg)

was missing the following properties:

  1. store

Generated by 🚫 dangerJS against 95c7b2e

This was referenced Mar 27, 2023
Closed
Closed
@ericanderson
Copy link
Contributor

I kept MutableRefObject around so that anyone using RefObject in their application code would not have the type change from under them and represented RefObject with MutableRefObject. It seemed like a cleaner deprecation path.

I also think if we are making breaking changes for React 19, we should not allow the zero argument version of useRef() since there are no documented uses in react. The behavior of useRef<T>() implying { current: T | undefined } is really confusing to most people when we inherit null in the other cases. I think either useRef<T>() needs to be { current: T | undefined | null } or needs to not exist. Both cover issue 2 in the referenced tweet.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 28, 2023

I also think if we are making breaking changes for React 19, we should not allow the zero argument version of useRef() since there are no documented uses in react.

We track this separately in #64920. This PR is not stacking all React 19 changes.

The behavior of useRef() implying { current: T | undefined } is really confusing to most people when we inherit null in the other cases.

Which other cases?

I think either useRef() needs to be { current: T | undefined | null } or needs to not exist.

{ current: T | undefined | null } will not be assignable to React managed refs since object properties are covariant in TypeScript. We cannot change that at the types declaration level. We need contravariant object properties for that: microsoft/TypeScript#21759 (comment)

#64920 will propose its non-existent i.e. only useRef<T>(undefined): { current: T < undefined } will exist (i.e. convenience overload for undefined just like null.

@eps1lon eps1lon mentioned this pull request Mar 29, 2023
Draft
@github-actions github-actions bot mentioned this pull request Jun 7, 2023
Merged
@eps1lon eps1lon force-pushed the feat/react/new-refs branch 3 times, most recently from 13f70c4 to c018004 Compare December 5, 2023 19:55
This was referenced Mar 26, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Igorbek Igorbek Awaiting requested review from Igorbek Igorbek will be requested when the pull request is marked ready for review Igorbek is a code owner

@johnnyreilly johnnyreilly Awaiting requested review from johnnyreilly johnnyreilly will be requested when the pull request is marked ready for review johnnyreilly is a code owner

@Jessidhia Jessidhia Awaiting requested review from Jessidhia Jessidhia will be requested when the pull request is marked ready for review Jessidhia is a code owner

Assignees
No one assigned
Labels
None yet
Projects
New Pull Request Status Board
Needs Author Action
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eps1lon @DangerBotOSS @ericanderson @gaearon