Skip to content

[react] forwardRefs ref object should be mutable #43265

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 1 commit into from
Mar 31, 2020
Merged

[react] forwardRefs ref object should be mutable #43265

merged 1 commit into from
Mar 31, 2020

Conversation

Pajn
Copy link
Contributor

@Pajn Pajn commented Mar 20, 2020

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

Fixes #39062

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Mar 20, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 20, 2020

@Pajn Thank you for submitting this PR!

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Please add a test that illustrates what you're fixing.

@Pajn
Copy link
Contributor Author

Pajn commented Mar 20, 2020

Added a test

@Pajn
Copy link
Contributor Author

Pajn commented Mar 20, 2020

And as usual recharts fails due to unrelated changes...

@typescript-bot typescript-bot added The Travis CI build failed Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Mar 20, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 20, 2020

@Pajn The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 20, 2020

@Pajn And you got some merge conflicts. Almost got a bingo 😄

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks for the test.

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. labels Mar 20, 2020
Please fill in this template.

- [x] Use a meaningful title for the pull request. Include the name of the package modified.
- [x] Test the change in your own code. (Compile and run.)
- [x] Add or edit tests to reflect the change. (Run with `npm test`.)
- [x] Follow the advice from the [readme](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#make-a-pull-request).
- [x] Avoid [common mistakes](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#common-mistakes).
- [x] Run `npm run lint package-name` (or `tsc` if no `tslint.json` is present).

Select one of these and delete the others:

If changing an existing definition:
- [x] Provide a URL to documentation or source code which provides context for the suggested changes: #31065, #39062
- [x] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
- [x] If you are making substantial changes, consider adding a `tslint.json` containing `{ "extends": "dtslint/dt.json" }`. If for reason the any rule need to be disabled, disable it for that line using `// tslint:disable-next-line [ruleName]` and not for whole package so that the need for disabling can be reviewed.

Fixes  #39062
@Pajn
Copy link
Contributor Author

Pajn commented Mar 20, 2020

Oh, that was fixed quickly. Nice!

Merge conflicts I can deal with, unrelated errors in unrelated packages due to definitely typed still not having reasonable dependency ranges are mentally hard though :)

@typescript-bot typescript-bot added Merge:Express and removed The Travis CI build failed Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. labels Mar 20, 2020
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@uniqueiniquity uniqueiniquity merged commit e37dc3f into DefinitelyTyped:master Mar 31, 2020
@typescript-bot
Copy link
Contributor

I just published @types/react@16.9.28 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts). Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@types/react] type Ref does not include MutableRefObject but probably should.
4 participants