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

Upgrade react-select to v3 #11621

Merged
merged 1 commit into from
Dec 5, 2022
Merged

Upgrade react-select to v3 #11621

merged 1 commit into from
Dec 5, 2022

Conversation

m1lt0n
Copy link
Contributor

@m1lt0n m1lt0n commented Nov 21, 2022

Summary

In the context of regular dependency upgrades, this PR upgrades react-select to version 3.0.3 (from v2.4 that we're currently on. Current version of react-select is 5.6, but we need to take a multi-step approach as there are lots of breaking changes and it will require a significant amount of effort to get to the latest version).

From version To version Changelog Notes
2.4.4 3.0.3 changelog The only breaking changes are applied in v3.0.0, while the rest of the versions are mostly fixes. The changes include: upgrade of library emotion used by react-select, multiple entrypoints (which affect how imports are done) and finally normalized values (I have a comment around that in the code) relevant to JedWatson/react-select#3416

Notes for QA review: This change affects multi-select (e.g. inviting emails to team) and some single-select (dropdown) components we have. Specific usages that I have tested for is invitation modal (users email) or add to channels, as well as retention policy in admin console, language selection in user settings, but there may be some more (I know of apps form that I wasn't able to test manually). Also, since our css explicitly mentions class names from this library, it makes sense to keep an eye for visual regressions around selects.

Ticket Link

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

Release Note

NONE

@m1lt0n
Copy link
Contributor Author

m1lt0n commented Nov 21, 2022

/e2e-test

@mattermod
Copy link
Contributor

@m1lt0n m1lt0n force-pushed the MM-48340 branch 3 times, most recently from 458f361 to f5a20b2 Compare November 22, 2022 15:20
@@ -219,7 +219,7 @@ describe('AppsFormComponent', () => {
<AppsForm {...props}/>
</Provider>,
);
expect(wrapper.find(Modal.Body).find('.react-select__single-value').text()).toEqual('Option3');
expect(wrapper.find(Modal.Body).find('div.react-select__single-value').text()).toEqual('Option3');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emotion adds a wrapper around the actual div that has the same classNames as the actual div containing the value, so the test had to become more specific.

}
return (v as EmailInvite).value;
}));
if (value) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A breaking change in v3 of react-select is that when there are no options (e.g. a multi-select that we have deleted all options) previously it would lead to an empty array, while now it becomes more consistent and becomes a null value (same as initial state).

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be exposing the webapp's ReactSelect package to plugins? I'm wondering what the impact is, on plugins that are currently using v2 of the package

Copy link
Contributor Author

@m1lt0n m1lt0n Nov 23, 2022

Choose a reason for hiding this comment

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

That's a good question. Currently we're not exposing it (in the sense that we eg expose luxon or other libraries). Are plugins making assumptions about react-select and its version? cc: @hmhealey probably you have some context around this?

In that case, any major upgrade of this package would cause issues (e.g. there is an even bigger change in v3 with the multiple entrypoints that leads to changing how we import react-select, in that case plugins would again have to change).

Those things would be an issue if the plugin does not rely(as a dependency) on their react-select versions, though right?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have any way of guaranteeing that a plugin doesn't depend on a specific version of react-select or any other dependency. Even if we're careful with major version increases, there's still no guarantee that a minor update doesn't break anything.

I'd like to hold off on adding any more shared dependencies until we have plugins using module federation eventually since that has some ability to control the versions of dependencies in a way that's safe. Right now, every package we expose through the window ends up being another spot we may accidentally introduce breaking changes between the web app and plugins

@m1lt0n
Copy link
Contributor Author

m1lt0n commented Nov 22, 2022

/e2e-test

@mattermod
Copy link
Contributor

@m1lt0n m1lt0n added Setup Cloud Test Server Setup a test server using Mattermost Cloud 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Nov 22, 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.

@m1lt0n m1lt0n self-assigned this Nov 22, 2022
@m1lt0n m1lt0n marked this pull request as ready for review November 22, 2022 15:54
@m1lt0n m1lt0n requested a review from a team as a code owner November 22, 2022 15:54
@m1lt0n m1lt0n requested review from hmhealey, mickmister and M-ZubairAhmed and removed request for a team November 22, 2022 15:54
@m1lt0n
Copy link
Contributor Author

m1lt0n commented Nov 29, 2022

@M-ZubairAhmed @mickmister I'd love any further feedback/approval on this

@mickmister
Copy link
Contributor

@m1lt0n I've deployed this PR with the cloud plugin for testing with plugins. I'm not seeing any issues when using the React Select components on this page: https://react-select-v3-plugin-test.test.mattermost.cloud/admin_console/plugins/plugin_com.mattermost.custom-attributes. LGTM 👍

@m1lt0n m1lt0n removed the 2: Dev Review Requires review by a core commiter label Dec 1, 2022
@m1lt0n m1lt0n assigned jgilliam17 and unassigned m1lt0n Dec 1, 2022
@m1lt0n m1lt0n requested review from jgilliam17 and removed request for M-ZubairAhmed December 1, 2022 09:02
@m1lt0n
Copy link
Contributor Author

m1lt0n commented Dec 1, 2022

@jgilliam17

Notes for QA review: This change affects multi-select (e.g. inviting emails to team) and some single-select (dropdown) components we have. Specific usages that I have tested for is invitation modal (users email) or add to channels, as well as retention policy in admin console, language selection in user settings, but there may be some more (I know of apps form that I wasn't able to test manually). Also, since our css explicitly mentions class names from this library, it makes sense to keep an eye for visual regressions around selects.

@jgilliam17
Copy link
Contributor

Thanks @m1lt0n for including testing instructions.
Tested, looks good to merge.
Verified multi select and single select fields in channels and sys. console - working as before.
E2E report - no PR related failures.

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Dec 2, 2022
@mm-cloud-bot
Copy link

Test server destroyed

@jgilliam17 jgilliam17 assigned m1lt0n and unassigned jgilliam17 Dec 2, 2022
@m1lt0n m1lt0n merged commit b516432 into mattermost:master Dec 5, 2022
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants