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

getWrappedInstance() broken in ^8.0.0 #534

Closed
Jessidhia opened this issue Oct 4, 2018 · 9 comments
Closed

getWrappedInstance() broken in ^8.0.0 #534

Jessidhia opened this issue Oct 4, 2018 · 9 comments
Labels
bug

Comments

@Jessidhia
Copy link

@Jessidhia Jessidhia commented Oct 4, 2018

The documentation in the README specifies that you need to call getWrappedInstance() twice; however, neither of the new building blocks used internally by withNamespaces (withI18n, which in turn uses withContext) implement getWrappedInstance(). Even if they did, this would have required 3 chained calls of getWrappedInstance(), not 2.

A better approach would probably be to drop the withRef system entirely and have an innerRef prop that is forwarded as ref in the HOC's render().

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 4, 2018

fully agree had no time yet to search for a solution...but yes...calling getWrappedInstance is no longer an options (out of the reasons you mention).

So if i get your approach right - just passing in a innnerRef function as prop and use that on the ref in rendering

@Jessidhia

This comment has been minimized.

Copy link
Author

@Jessidhia Jessidhia commented Oct 4, 2018

Yeah, it doesn't even have to be a function; react 16.3+'s createRef object should do too.

It's similar to the approach taken by styled-components/emotion.

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 4, 2018

ok...will need to check that...

@Jessidhia

This comment has been minimized.

Copy link
Author

@Jessidhia Jessidhia commented Oct 4, 2018

Even better would be forwardRef but that would require React 16.4+ and at least react-redux had some performance concerns about it: facebook/react#13456

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 4, 2018

seems you know a lot more about refs then me 💪i just try to avoid them personally

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 4, 2018

@jamuhl jamuhl added the bug label Oct 4, 2018
@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 4, 2018

landed in react-i18next@8.0.7

@jamuhl jamuhl closed this Oct 4, 2018
yuki-takei added a commit to weseek/growi that referenced this issue Oct 4, 2018
* react-dropzone
* react-i18next
    * coudn't upgrade to v8 because of 'getWrappedInstance()`
    * i18next/react-i18next#534
    * https://react.i18next.com/deprecated/translate-hoc
* webpack-bundle-analyzer
@Jessidhia

This comment has been minimized.

Copy link
Author

@Jessidhia Jessidhia commented Oct 17, 2018

The fix did fix withNamespaces, but withI18n and withContext still have no way to forward the ref to the wrapped component.

Moving the innerRef implementation to withContext's HOC should be sufficient to get support for all 3.

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 17, 2018

@Kovensky should be in react-i18next@8.0.8

@Jessidhia Jessidhia mentioned this issue Oct 17, 2018
7 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.