Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[MM-34187] - Update React version to 17* #10739

Merged
merged 8 commits into from
Jul 21, 2022
Merged

[MM-34187] - Update React version to 17* #10739

merged 8 commits into from
Jul 21, 2022

Conversation

nevyangelova
Copy link
Contributor

Summary

Upgrade React to 17

Ticket Link

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

Release Note

NONE

@nevyangelova nevyangelova added 2: Dev Review Requires review by a core commiter QA Deferred Agreement with QA that these changes will be tested post-merge labels Jul 8, 2022
@mattermod
Copy link
Contributor

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

@M-ZubairAhmed
Copy link
Member

@nevyangelova should we keep the QA review. Just in case to run e2e test and a smoke test of the app to see if stuff broke?

zefhemel
zefhemel approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@zefhemel zefhemel left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll approve this to have the e2e tests run.

@zefhemel zefhemel added QA Deferred Agreement with QA that these changes will be tested post-merge and removed QA Deferred Agreement with QA that these changes will be tested post-merge labels Jul 12, 2022
@mattermod
Copy link
Contributor

E2E tests not automatically triggered, because the PR is not in a mergeable state. Please update the branch with the base branch and resolve outstanding conflicts.

@zefhemel
Copy link
Contributor

@nevyangelova sadly, there's a conflicting package.json now.

@zefhemel zefhemel removed the request for review from M-ZubairAhmed July 14, 2022 08:42
@nevyangelova
Copy link
Contributor Author

@hmhealey kind reminder to review :)

hmhealey
hmhealey suggested changes Jul 18, 2022
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Could you also update react-is and @hot-loader/react-dom? If I understand correctly, those have to be updated at the same times as these other ones.

Also, could you update the file that tracks bigger dependency updates (.npmupgrade) to point to MM-45255 as the ticket for the next round of React upgrades? There's a bunch of entries that refer to this ticket since those were for React 17

@hmhealey hmhealey added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jul 18, 2022
@hmhealey
Copy link
Member

/e2e-tests

@mattermod
Copy link
Contributor

Successfully triggered e2e testing!
https://git.internal.mattermost.com/qa/cypress-ui-automation/-/pipelines/219978

@M-ZubairAhmed M-ZubairAhmed added 3: QA Review Requires review by a QA tester and removed QA Deferred Agreement with QA that these changes will be tested post-merge labels Jul 19, 2022
@mattermod
Copy link
Contributor

E2E tests not automatically triggered, because the PR is not in a mergeable state. Please update the branch with the base branch and resolve outstanding conflicts.

M-ZubairAhmed
M-ZubairAhmed reviewed Jul 19, 2022
View reviewed changes
package.json Outdated
@@ -16,12 +16,12 @@
"@stripe/react-stripe-js": "1.6.0",
"@stripe/stripe-js": "1.20.3",
"@tippyjs/react": "4.2.6",
"@types/color-hash": "1.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

0/5 can we move it dev dependencies, since all our @types are there

M-ZubairAhmed
M-ZubairAhmed reviewed Jul 19, 2022
View reviewed changes
Copy link
Member

@M-ZubairAhmed M-ZubairAhmed left a comment

Choose a reason for hiding this comment

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

As @hmhealey have mentioned we also need to remove react and react dom from .npm-upgrade.json

@nevyangelova
Copy link
Contributor Author

@hmhealey @M-ZubairAhmed updated

hmhealey
hmhealey reviewed Jul 20, 2022
View reviewed changes
package.json
@@ -153,7 +153,7 @@
"css-loader": "5.2.6",
"dotenv-webpack": "7.0.3",
"enzyme": "3.11.0",
"enzyme-adapter-react-16": "1.15.6",
"enzyme-adapter-react-17-updated": "1.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize Enzyme didn't support React 17 (enzymejs/enzyme#2429) or 18 (enzymejs/enzyme#2524) itself, and that it hasn't been updated in quite a while (the last release of Enzyme was in December 2019). I'm going to file a ticket to start investigating switching away from Enzyme since it seems like it's no longer being properly supported

Copy link
Member

Choose a reason for hiding this comment

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

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

@hmhealey
Copy link
Member

@nevyangelova @jgilliam17 When we do QA review on this, would you mind spending some time trying to break the LHS menus and the drag and drop in it? I still can't remember exactly what the issue was with React 17 when I previously tried to update, but I think it was one of those two things

@nevyangelova nevyangelova added the QA Deferred Agreement with QA that these changes will be tested post-merge label Jul 20, 2022
@mattermod
Copy link
Contributor

Successfully triggered e2e testing!
https://git.internal.mattermost.com/qa/cypress-ui-automation/-/pipelines/220855

M-ZubairAhmed
M-ZubairAhmed approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@M-ZubairAhmed M-ZubairAhmed left a comment

Choose a reason for hiding this comment

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

Big change coming in wohoooo :D

@nevyangelova
Copy link
Contributor Author

@nevyangelova @jgilliam17 When we do QA review on this, would you mind spending some time trying to break the LHS menus and the drag and drop in it? I still can't remember exactly what the issue was with React 17 when I previously tried to update, but I think it was one of those two things

tested, worked as expected for me.

@nevyangelova nevyangelova merged commit 8ca0eee into master Jul 21, 2022
@nevyangelova nevyangelova deleted the MM-34187 branch July 21, 2022 16:04
@mattermod mattermod removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jul 21, 2022
@mm-cloud-bot
Copy link

Test server destroyed

@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jul 22, 2022
neallred added a commit that referenced this pull request Jul 22, 2022
@neallred
Revert "[MM-34187] - Update React version to 17* (#10739)"
0bf9ced 
This reverts commit 8ca0eee.
neallred added a commit that referenced this pull request Jul 22, 2022
@neallred
Revert "[MM-34187] - Update React version to 17* (#10739)" (#10823)
f8cd6f0 
This reverts commit 8ca0eee.
@hmhealey hmhealey mentioned this pull request Jul 25, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hmhealey hmhealey hmhealey approved these changes

@M-ZubairAhmed M-ZubairAhmed M-ZubairAhmed approved these changes

@zefhemel zefhemel zefhemel approved these changes

Assignees
No one assigned
Labels
2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation QA Deferred Agreement with QA that these changes will be tested post-merge release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nevyangelova @mattermod @M-ZubairAhmed @zefhemel @hmhealey @mm-cloud-bot @amyblais