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

PLAT-72712 - i18n: lifecycle method updates per locale prop update #2089

Merged
merged 6 commits into from
Feb 4, 2019

Conversation

rundmt
Copy link
Contributor

@rundmt rundmt commented Jan 31, 2019

Checklist

  • I have read and understand the contribution guide

  • A CHANGELOG entry is included

  • At least one test case is included for this feature or bug fix

  • Documentation was added or is not needed

  • This is an API breaking change

Issue Resolved / Feature Added

Needed to update i18n to use new lifecycle methods

Resolution

Updated to use safe lifecycle methods

Additional Considerations

Originally I tried to make make use of getDerivedStateFromProps, but using async things in there is not recommended. So I ended up putting it in componentDidUpdate.
reactjs/rfcs#26

Links

PLAT-72712

JayCanuck and others added 5 commits January 11, 2019 16:39
Enact-DCO-1.0-Signed-off-by: Derek Tor <derek.tor@lge.com>
Enact-DCO-1.0-Signed-off-by: Derek Tor <derek.tor@lge.com>
Copy link
Contributor

@ryanjduffy ryanjduffy left a comment

Choose a reason for hiding this comment

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

👍 Good work, @rundmt! Even though we're updating state in componentDidUpdate, I think this enforces the correct order of operations as originally implemented in the async ilib updates. It would be possible to update state in {{getDerivedStateFromProps}} but that would cause the state to be out of sync with ilib's internal state. By deferring to componentDidUpdate, we ensure that the internal state update doesn't occur until after ilib has updated (either synchronously or asynchronously).

Copy link
Contributor

@viodragon2 viodragon2 left a comment

Choose a reason for hiding this comment

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

👍

@viodragon2 viodragon2 merged commit e57b358 into feature/PLAT-53066 Feb 4, 2019
@viodragon2 viodragon2 deleted the feature/PLAT-72712 branch February 4, 2019 19:22
@viodragon2 viodragon2 restored the feature/PLAT-72712 branch February 4, 2019 19:24
@viodragon2 viodragon2 deleted the feature/PLAT-72712 branch February 4, 2019 19:30
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.

4 participants