MM-17555 Finish upgrading react-intl #4615

hmhealey · 2020-01-03T18:28:15Z

This is the final PR to update to the new version of react-intl. Most of it should be nonfunctional stuff like updating snapshots and tests, but there's a few important things I wanted to call out:

The biggest change that I didn't expect is that the version of react-bootstrap's OverlayTrigger that we use doesn't pass context correctly, so I had to make a custom version of OverlayTrigger to make it so that modals and popovers (like the profile popover) could be translated correctly. This change was merged as a separate PR.
This is using @cometkim's new versions of shallowWithIntl and mountWithIntl, but I removed the step of them that unwraps calls to React.forwardRef since it wasn't necessary and it caused some problems when we were trying to test components that used forwardRef when it wasn't from injectIntl
Related to the above, shallowWithIntl is only needed for testing components that use injectIntl, so I've removed it from all tests where it wasn't needed. Similarly, we no longer need to .dive into most components when testing them. This change was merged in a separate PR.
React-intl now uses React.forwardRef to pass refs into injectIntl components. This was the big source of issues last time that we attempted this upgrade, but I think I've gone through and either tested or removed everywhere we did this

Ticket Link

https://mattermost.atlassian.net/browse/MM-17555

enahum · 2020-01-07T12:48:53Z

@hmhealey can you push this PR to be tested first before I do the review, it is a very large PR and I would rather review it after having confirmation that nothing is broken.

cometkim · 2020-01-08T13:45:39Z

Not only OverlayTrigger, but also Tooltips have same context issue as well. it completely destroys the context API usage and causes memory leaks in VDOM. Wouldn't it be better to upgrade react-bootstrap first, even if it spends more time?

hmhealey · 2020-01-08T16:03:07Z

@enahum I'll see if I can do that, although we're in the release cycle right now, so it might be difficult. If it can't be reviewed right now, I'll look at breaking up this PR some more.

@cometkim I could implement a custom Tooltip replacement like OverlayTrigger, but what do you mean by it causing memory leaks? I wasn't aware of anything like that

cometkim · 2020-01-08T17:54:37Z

@hmhealey never mind. I suspected that because elements are duplicated in the inspector and never removed. It seems to be a bug in the inspector.

hmhealey · 2020-01-08T22:08:14Z

I'm going to break this up into a few more PRs wherever possible. I'll re-add reviewers once those PRs are in

hmhealey · 2020-01-08T22:28:12Z

@cometkim Have you found any specific instances where the Tooltip isn't passing the context correctly? It seems to be working fine everywhere I've checked

hmhealey · 2020-01-10T17:37:14Z

This is ready for review again now that some of the large, distracting changes have been merged separately.

For dev reviewers, if you filter out the changes to the .snap files in GitHub, it'll make your live's a lot easier since there's a lot of renaming going on caused by react-intl renaming InjectIntl to injectIntl as the component's display name

jespino

LGTM

jespino · 2020-01-11T16:50:56Z

@hmhealey test are failing, probably related to the merge done

mattermod · 2020-01-13T19:39:40Z

Mattermost test server updated with git commit 4bd5bd9fadea827a4f46f25a63337cf202bd8ba6.

Access here: https://mattermost-webapp-pr-4615.test.mattermost.cloud

enahum

LGTM, one question as it seems weird, but not a blocker as long as it works

enahum · 2020-01-16T13:33:58Z

components/create_comment/create_comment.jsx

@@ -704,8 +704,8 @@ class CreateComment extends React.PureComponent {
 if (index !== -1) {
 uploadsInProgress.splice(index, 1);

- if (this.refs.fileUpload && this.refs.fileUpload.getWrappedInstance() && this.refs.fileUpload.getWrappedInstance().getWrappedInstance()) {
- this.refs.fileUpload.getWrappedInstance().getWrappedInstance().cancelUpload(id);
+ if (this.refs.fileUpload && this.refs.fileUpload.getWrappedInstance()) {


do you need getWrappedInstance even by using forwardRef ? that seems strange

The remaining getWrappedInstance is actually from connect. If that gets updated to use React.forwardRef, then we can remove the second getWrappedInstance

enahum · 2020-01-16T13:39:47Z

@hmhealey PR needs updating, @jgilliam17 please try and test the PR soon ;)

jgilliam17

Tested, looks good to merge.

Verified Localizations, checked popovers and modals opened by the app
Verified invalid slash command trigger error
Verified using Alt + arrows to switch channels
Verified profile popover opens as expected
Able to log in/logout without any issues

mattermod · 2020-01-16T15:00:37Z

Test server destroyed

hmhealey added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 3, 2020

hmhealey added this to the v5.20.0 milestone Jan 3, 2020

hmhealey requested review from jespino, enahum and jgilliam17 January 3, 2020 18:28

hmhealey added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 8, 2020

hmhealey removed request for jespino, enahum and jgilliam17 January 8, 2020 22:07

hmhealey added Work in Progress Not yet ready for review and removed Work in Progress Not yet ready for review labels Jan 8, 2020

This was referenced Jan 8, 2020

MM-17555 Only use shallowWithIntl and dive when necessary #4637

Merged

MM-17555 Update tests that use string refs #4638

Merged

hmhealey mentioned this pull request Jan 8, 2020

MM-17555 Add dummy OverlayTrigger component #4639

Merged

hmhealey added 7 commits January 9, 2020 15:08

MM-17555 Update react-intl

aafe9eb

Update most snapshots that just rename injectIntl

212c113

Fix tests that manually create an intl object

4e0d8b7

Update misc snapshots

61ecaa7

Fix tests for UserSettingsGeneral

843da86

Replace usage of withRef with forwardRef

1faa13e

Implement custom OverlayTrigger to pass intl context to overlays

e4f3919

hmhealey added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 10, 2020

hmhealey requested review from jespino, enahum and jgilliam17 January 10, 2020 17:35

Merge branch 'master' into mm17555-final

e1b39fa

jespino approved these changes Jan 11, 2020

View reviewed changes

jgilliam17 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jan 13, 2020

hmhealey added 4 commits January 13, 2020 12:32

Merge branch 'master' into mm17555-final

f401ccf

Update tests for RecentDate

30ba469

Merge branch 'master' into mm17555-final

1c471fb

Merge branch 'master' into mm17555-final

4bd5bd9

enahum approved these changes Jan 16, 2020

View reviewed changes

jgilliam17 approved these changes Jan 16, 2020

View reviewed changes

jgilliam17 added QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jan 16, 2020

hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Jan 16, 2020

Merge branch 'master' into mm17555-final

8a433b5

hmhealey merged commit 7b65d4e into master Jan 16, 2020

hmhealey deleted the mm17555-final branch January 16, 2020 20:15

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 17, 2020

cometkim mentioned this pull request Feb 29, 2020

MM-19982: Fix error on trigger ProfilePopover #4185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-17555 Finish upgrading react-intl #4615

MM-17555 Finish upgrading react-intl #4615

hmhealey commented Jan 3, 2020 •

edited

Loading

enahum commented Jan 7, 2020

cometkim commented Jan 8, 2020

hmhealey commented Jan 8, 2020

cometkim commented Jan 8, 2020

hmhealey commented Jan 8, 2020

hmhealey commented Jan 8, 2020

hmhealey commented Jan 10, 2020

jespino left a comment

jespino commented Jan 11, 2020

mattermod commented Jan 13, 2020

enahum left a comment

enahum Jan 16, 2020

hmhealey Jan 16, 2020

enahum commented Jan 16, 2020

jgilliam17 left a comment

mattermod commented Jan 16, 2020

MM-17555 Finish upgrading react-intl #4615

MM-17555 Finish upgrading react-intl #4615

Conversation

hmhealey commented Jan 3, 2020 • edited Loading

Ticket Link

enahum commented Jan 7, 2020

cometkim commented Jan 8, 2020

hmhealey commented Jan 8, 2020

cometkim commented Jan 8, 2020

hmhealey commented Jan 8, 2020

hmhealey commented Jan 8, 2020

hmhealey commented Jan 10, 2020

jespino left a comment

Choose a reason for hiding this comment

jespino commented Jan 11, 2020

mattermod commented Jan 13, 2020

enahum left a comment

Choose a reason for hiding this comment

enahum Jan 16, 2020

Choose a reason for hiding this comment

hmhealey Jan 16, 2020

Choose a reason for hiding this comment

enahum commented Jan 16, 2020

jgilliam17 left a comment

Choose a reason for hiding this comment

mattermod commented Jan 16, 2020

hmhealey commented Jan 3, 2020 •

edited

Loading