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

Typescript #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Typescript #35

wants to merge 1 commit into from

Conversation

nayaabkhan
Copy link
Owner

No description provided.

@ctavan
Copy link

ctavan commented Oct 16, 2019

@nayaabkhan I see you have started a Typescript-Rewrite.

I do have some interest in getting rid of the componentWillReceiveProps deprecation warning so I was wondering if you are palling to include that change in here (similar to #32)?

I'd also be happy to help, I was just wondering whether it made sense at all to supply a patch for this based on the current JavaScript develop branch or whether I should rather wait for you to finish the TypeScript rewrite?!

@nayaabkhan
Copy link
Owner Author

@ctavan I think it is better to patch the componentWillReceiveProps in a separate PR from the current develop branch. Feel free to do that, TypeScript migration isn't something that should block it.

The commit at c5c6fac is pretty much all we need but maybe if we can trim it down to only the substitution of componentWillReceiveProps it would be easier to move it ahead.

@ctavan
Copy link

ctavan commented Oct 16, 2019

@nayaabkhan I just realized that in 9210c44 componentWillReceiveProps was replaced by componentDidUpdate.

This means that with 0.6.0 the react deprecation warning is gone. I'm still a bit concerned whether componentDidUpdate is the right lifecycle method to update the polyglot instance (i.e. whether this will propagate the changes down the component tree before it has been re-rendered). Do you know if this has been tested?

Also, what was confusing me in the first place is that the current master branch of this repo does not even contain the latest release of this library (which I then found in develop), maybe either make the develop branch the default on github or go back to master?!

@nayaabkhan
Copy link
Owner Author

@ctavan Ah yes, I forgot to bring master up to speed with develop since my last pushes. I just did that, sorry for the confusion.

About componentDidUpdate, it is indeed something to be verified. I will look into it and if required, patch it.

@nayaabkhan
Copy link
Owner Author

@ctavan Thanks for pointing out about the componentDidUpdate, it was indeed causing the bug where the instance was not in sync with the new props. I have pushed a patch.

@ctavan
Copy link

ctavan commented Oct 16, 2019

Thanks! Which means we still need a different way of doing this in order to remain compatible with react>=17

@soda0289
Copy link

soda0289 commented Oct 17, 2019

@ctavan @nayaabkhan

The issue with componentDidUpdate is that although we do update the polyglot instance it only happens after we have rendered.

Is there any reason why we cant just do the check inside render().

Something like

if (this.polyglot.currentLocale  !== this.props.locale) {
  this.polyglot.locale(this.props.locale);
}

That way we update the instance then create a new bound function that would trigger a re render of the Context consumers.

I can work on this change as well since we have run into issues where changing locale wont trigger re render.

**EDIT: The workaround we use now is to pass in a key prop for <I18n /> component that uses the locale.
<I18n key={locale} locale={locale} />

@ctavan
Copy link

ctavan commented Oct 18, 2019

@soda0289 I took this over to #38 to not further hijack this TypeScript pull request, let's continue the discussion over there.

@nayaabkhan nayaabkhan changed the base branch from develop to master June 17, 2021 07:40
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