mattermost / mattermost-plugin-starter-template

Build scripts and templates for writing Mattermost plugins.
https://developers.mattermost.com/extend/plugins/
Apache License 2.0
128 stars 120 forks source link

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

Closed hmhealey closed 1 year ago

hmhealey commented 1 year ago

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

https://github.com/mattermost/mattermost-plugin-playbooks/pull/1489 https://github.com/mattermost/focalboard/pull/3861 https://github.com/mattermost/mattermost-plugin-todo/pull/190 https://github.com/mattermost/mattermost-plugin-demo/pull/153

mickmister commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

/update-branch

hmhealey commented 1 year ago

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