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

Api modernization #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

publicJorn
Copy link

Hi @nayaabkhan,

Here's a PR that does the following:

  • Removed componentWillReceiveProps so it's future proof. Kept the class layout in favour to function/hook layout, so polyglot can still be bound to the instance.
  • Moved the this._polyglot calls inside an if, assuming messages will only need to change when locale changes. This is a bit more efficient.
  • Introduced new prop forceReInit that negates above check (mimicks old default behaviour)
  • More tests

The second commit is a personal thing, but I think it's neater. Instead of calling translate()(Component) you can now just call translate(Component). There's no initialisation going on in I18n anyway.

It's a breaking change, so I can remove it from this PR if you don't like it.

@@ -17,7 +17,7 @@ export interface TranslateProps {
}

/** Wrap your components with `translate` to get a prop `t` passed in. */
declare const translate = () => <T extends object>(
declare const translate = <T extends object>(
Copy link

Choose a reason for hiding this comment

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

This should go into the other commit!

src/translate.js Outdated
{t => <WrappedComponent {...props} t={t} />}
</I18nContext.Consumer>
)
export default function translate()(WrappedComponent) {
Copy link

Choose a reason for hiding this comment

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

This doesn't look like valid javascript… Mixed up with the other commit?!

@ctavan
Copy link

ctavan commented Sep 5, 2019

@nayaabkhan can we get the componentWillReceiveProps removal commit?

I personally don't think there's a need to introduce a breaking API change (second commit) at this point, but it would be nice to get the modernizations in place.

@publicJorn I believe the first commit contains a few changes related to your api change, see inline comments… Would you mind having a second look? Or maybe just open another pullrequest with just the code modernization without the breaking change to increase chances we get this merged quickly? Thanks for your work!

@publicJorn
Copy link
Author

Thanks @ctavan for the review. I fixed translate. Don't know how the tests passed, they didn't when I checked locally. But now they do.
Checked the example after build, it also runs.

I discarded the api change.

@nayaabkhan are you still managing this project? I see some other PRs have been open for some time now.

Myself, I've moved to i18next, because I needed more features than polyglot could offer. But I'll leave this PR open.

@nayaabkhan
Copy link
Owner

@publicJorn Thanks for your PR. Yes, I do maintain this project. Unfortunately, I cannot put myself dedicatedly on this project as I work full time elsewhere. But I make sure to keep maintaining this project during the weekends.

@ctavan ctavan mentioned this pull request Oct 16, 2019
@ctavan
Copy link

ctavan commented Oct 16, 2019

The deprecation warning was also removed in 9210c44 and no longer appears in [email protected]

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.

3 participants