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

#1211 - Deprecate withRef in favor of forwardRef and React.forwardRef #1271

Merged
merged 2 commits into from May 27, 2019

Conversation

mrijke
Copy link
Contributor

@mrijke mrijke commented May 3, 2019

Deprecate withRef and use forwardRef prop instead.
When forwardRef is true, the ref passed to the injected component
will be passed down to the wrapped component.

Display a invariant message when withRef is used.

Closes #1211.

Since this is a breaking change it would be nice to have this for 3.0 :)

@yahoocla
Copy link

yahoocla commented May 3, 2019

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@redonkulus
Copy link
Member

@mrijke Did you sign the CLA?

@DragonRaider5 care to review this?

@marcesengel
Copy link
Collaborator

Hey @mrijke,

first of all: thanks a lot for your contribution! 👍 🎉
Also thanks @redonkulus for bringing this to my attention.

I'll now do a small review and edit here if something comes to my mind which doesn't fit into the review.

Copy link
Collaborator

@marcesengel marcesengel left a comment

Choose a reason for hiding this comment

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

Thanks again for your work.
I'd be happy to discuss the changes that came to my mind, don't take them as "required".

In addition I'd love to help you change what I asked about if time doesn't allow you to do so.

src/components/withIntl.js Outdated Show resolved Hide resolved
src/components/withIntl.js Outdated Show resolved Hide resolved
src/components/withIntl.js Outdated Show resolved Hide resolved
test/unit/components/withIntl.js Show resolved Hide resolved
test/unit/components/withIntl.js Outdated Show resolved Hide resolved
@mrijke
Copy link
Contributor Author

mrijke commented May 16, 2019

@redonkulus I did sign the CLA, is there anything I need to do to retrigger the check?

@marcesengel
Copy link
Collaborator

Now that react-intl isn't part of the Yahoo organisation any more, can Yahoo even be held liable for anything?
If not, can we just drop the CLA?

@redonkulus
Copy link
Member

@DragonRaider5 I've removed the CLA webhook, so its ok to ignore now.

Deprecate withRef and use forwardRef prop instead.
When forwardRef is true, the ref passed to the injected component
will be passed down to the wrapped component.

Display a invariant message when withRef is used.

Closes formatjs#1211.
@mrijke mrijke force-pushed the feat-1211/forwardRef branch 2 times, most recently from 38849aa to 798a17a Compare May 20, 2019 06:09
@redonkulus redonkulus added this to the v3.0.0 milestone May 24, 2019
Copy link
Collaborator

@marcesengel marcesengel left a comment

Choose a reason for hiding this comment

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

Hey, it's me once again 😆

I think this should be the last revision, just some tweaks to make the code easier to understand and one question.
Thanks in advance and have a nice weekend!

package.json Outdated Show resolved Hide resolved
src/components/withIntl.js Outdated Show resolved Hide resolved
Refactor the call to withIntl to allow for HoC composing, such as:

export default compose(
  connect(...conntextOptions),
  withIntl(intlOptions)
)

This is done without changing the main API, options can still be passed
in as second arguments to withIntl too.

Add 'react-is' as a strict dependency, change the rollup config to be
able to properly build with it.
Copy link
Collaborator

@marcesengel marcesengel left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution over the past weeks 🎉

To me this looks great, @redonkulus what do you think about merging?

@redonkulus redonkulus merged commit 6c36e74 into formatjs:master May 27, 2019
@redonkulus
Copy link
Member

Released in v3.0.0-beta-4!

@mrijke mrijke deleted the feat-1211/forwardRef branch May 27, 2019 16:14
@dkhizhniakov
Copy link

dkhizhniakov commented Nov 26, 2019

Hi!
I think that example of usage should be included into the docs. It looked misleading for me after using previous version with "withRef" option.

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.

Any plans to use forwardRef with injectIntl?
5 participants