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

MM-47046 Use version of React DOM provided by web app #169

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

hmhealey
Copy link
Member

@hmhealey hmhealey commented Sep 16, 2022

For some more context, see here. The short version though is that we updated the web app to React 17, and there's a chance that plugins will have some issues with it because they're compiled with the React 16 version of ReactDOM. I'm submitting PRs to the 3 products, the demo plugin, and the plugin template to have them use the web app's version of React DOM to fix any immediate issues, but we'll want to properly migrate them to React 17 going forward.

Ticket Link

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

Related Pull Requests

mattermost/mattermost-plugin-playbooks#1489
mattermost/focalboard#3861
mattermost-community/mattermost-plugin-todo#190
mattermost/mattermost-plugin-demo#153

@hmhealey hmhealey added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Sep 16, 2022
@hmhealey hmhealey changed the title MM-47076 Use version of Reaact DOM provided by web app MM-47046 Use version of Reaact DOM provided by web app Sep 16, 2022
@hmhealey hmhealey force-pushed the MM-47052_use-web-app-react-dom branch from ddc906c to eb07d71 Compare September 16, 2022 19:36
@hmhealey hmhealey changed the title MM-47046 Use version of Reaact DOM provided by web app MM-47046 Use version of React DOM provided by web app Sep 16, 2022
@mickmister
Copy link
Member

During this sweep of updates, I'm wondering if we should also include other packages that are available on window:

https://github.com/mattermost/mattermost-webapp/blob/7148a482b8c01ae1df99332730f9dc51d3d34659/plugins/export.js#L29-L38

window.React = require('react');
window.ReactDOM = require('react-dom');
window.ReactIntl = require('react-intl');
window.Redux = require('redux');
window.ReactRedux = require('react-redux');
window.ReactBootstrap = require('react-bootstrap');
window.ReactRouterDom = require('react-router-dom');
window.PropTypes = require('prop-types');
window.Luxon = require('luxon');
window.StyledComponents = require('styled-components');

@mickmister
Copy link
Member

I'm wondering if we should also update package.json to have React 17, to make sure any existing libraries in the individual plugin projects are updated to follow relevant breaking changes from the React library. I wonder what sort of backwards compatbilities this may cause on a server running React 16 if we go down this route.

@hmhealey hmhealey removed the 2: Dev Review Requires review by a core committer label Sep 30, 2022
@hmhealey
Copy link
Member Author

I'm 0/5 on whether or not we want to include those other externals as well. I don't know if adding them to externals means that we need to include them in the package.json or not.

As for updating the actual version, that's probably a good idea, but it will 100% cause dependency conflicts because of how we import mattermost-webapp, so I'd prefer to spin that off into a separate ticket. We need to resolve that issue as well, but it's likely going to be a pain because we're in a bit of dependency hell here.

@hanzei
Copy link
Contributor

hanzei commented Oct 7, 2022

/update-branch

@hmhealey
Copy link
Member Author

hmhealey commented Nov 2, 2022

Skipping QA review on this since it's a straightforward enough fix that we've already applied to a bunch of other repos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants