react-polyglot icon indicating copy to clipboard operation
react-polyglot copied to clipboard

Published 20 hours ago • verified publisher icon nayaabkhan

Api modernization

Open publicJorn opened this issue 5 years ago • 4 comments

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.

publicJorn avatar Aug 26 '19 11:08 publicJorn

@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!

ctavan avatar Sep 05 '19 08:09 ctavan

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.

publicJorn avatar Sep 30 '19 08:09 publicJorn

@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.

nayaabkhan avatar Sep 30 '19 13:09 nayaabkhan

The deprecation warning was also removed in https://github.com/nayaabkhan/react-polyglot/commit/9210c448b07c5810b97a162e297dc100c3959036 and no longer appears in [email protected]

ctavan avatar Oct 16 '19 09:10 ctavan