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

#1047 Hoist non-react statics in injectIntl #1141

Merged
merged 1 commit into from Sep 6, 2018

Conversation

bringking
Copy link
Contributor

@bringking bringking commented Jun 20, 2018

Resolves #1047. We were running into an issue locally using react-intl with NextJS, which has a static called getInitialProps. This PR adds hoists statics using hoist-non-react-statics in the injectIntl HOC and adds a test. Thanks for the great library!

@bringking
Copy link
Contributor Author

bringking commented Jun 20, 2018

That test failure seems unrelated, I can't retry the build though...

@bringking
Copy link
Contributor Author

I would love to get this landed sometime soon.. is this still maintained?

@bringking
Copy link
Contributor Author

@ericf are you the maintainer? Let me know what I can do to help get this in 🙏

@bringking
Copy link
Contributor Author

Is this library maintained anymore? Anything I can do to help?

@papasmile
Copy link
Contributor

@redonkulus is last known committer...

@redonkulus
Copy link
Member

@okuryu is the most recent maintainer. I have commit access and I merge simple changes, but I'm not an active maintainer. We are definitely looking for more maintainers to continue to move this project along. For any new code changes, it just needs to be properly tested. I've restarted the failing build to see if it was a one off issue.

@redonkulus
Copy link
Member

As for this PR, the original owner of this library had a lengthy discussion with other folks on why injectIntl does not hoist statics. Its worth reviewing those comments to understand why this was never merged previously.

@bringking
Copy link
Contributor Author

bringking commented Jul 9, 2018

@okuryu @redonkulus @ericf yeah I understand the original discussion and it makes some sense. A library shouldn't concern itself with non-react statics per se. All the problems with HOC's aside, it seems strange to take a stand on this and break expected HOC behavior from many different libs in the ecosystem. Now we have a foot-gun in our repo that we have to handle with a special case... I disagree with the original authors assertion that it is "brittle". hoist-non-react-statics has been in wide use for quite some time. Can we re-visit merging this to be a good citizen with the HOC?

@papasmile
Copy link
Contributor

@redonkulus I think you have a volunteer: #1160 (comment)

@redonkulus
Copy link
Member

Thanks for the bump. I'm ok with the change, assuming it won't break anyone if its gets released. @sourabh2k15 has volunteered to help maintain this project too. I'd like to see him review and address any concerns or questions.

@redonkulus
Copy link
Member

Given the changes and the expectation in the community, I'm ok with merging this.

@kelly-tock
Copy link

Yes!!!!!!!!

@sourabh2k15
Copy link
Contributor

@redonkulus yes I would be contributing more actively from now onwards. I'll help reviewing the other PRs

@Sparragus
Copy link

I'm very happy this was merged. Thanks @redonkulus for allowing this to go in.

@backmeupplz
Copy link

soundsgood
v2.4.0 — I have to hoist my non-react statics back to component every time, even with my enormously huge hands. We already live in 2k18, it's a shame that some libraries don't hoist the components — and even when they merge it's just fake news. Please, make sure it works. We really need it.

@Sparragus
Copy link

@backmeupplz Hey. Be more respectful to the maintainers.

@backmeupplz
Copy link

@Sparragus no offence intended, sorry if I insulted anybody.

@backmeupplz
Copy link

Also, did yarn cache clean and re-installed the node_modules. Was testing the theory that this commit was merged to 2.4.0 without the version bump and yarn cached an older version. Didn't work. Will have to investigate further. Manually hoisting the components works.

wiki-background

@bringking
Copy link
Contributor Author

bringking commented Sep 11, 2018

@backmeupplz if I am reading correctly, this wasn't released yet. 2.4.0 was released in Sept. of 2017 - https://github.com/yahoo/react-intl/releases/tag/v2.4.0

@backmeupplz
Copy link

@bringking oh ok, I'll use master branch then :) thx

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.

None yet

7 participants