Added the mattermost-webapp from the monorepo of mattermost #1074
Conversation
Makefile
Outdated
There was a problem hiding this comment.
These seem like unrelated changes
There was a problem hiding this comment.
I was facing a very peculiar error https://github.com/mattermost/mattermost-plugin-jira/actions/runs/8938919453/job/24553976441, so there was a package named file-hound of frontend, and after it was installed, it was creating an empty go file, and in the lint as well as test CI, we run commands like go test ./... which basically going to work on all files including the node modules and that's why we were getting the above error. So for now I have made the go commands to only work in the server folder and not outside of it.
@hanzei
There was a problem hiding this comment.
I think the problem @hanzei may have with this is that the build folder is no longer being checked. Can we instead make it so only webapp exists in the monorepo checkout folder? Either we can do something replacing the whole repo with just the webapp directory like:
mv mattermost-webapp/webapp .
rm -rf mattermost-webapp
mv webapp mattermost-webappNaming the cloned directory just mattermost may make more sense here. Then this becomes:
mv mattermost/webapp .
rm -rf mattermost
mv webapp mattermost-webappAlternatively we can do a sparse checkout, but that seems more complicated https://stackoverflow.com/a/52270636
There was a problem hiding this comment.
Nice work on this @Kshitij-Katiyar 🚀
In general LGTM 👍 Just some comments for discussion, and one request to move the monorepo commit hash to package.json to keep all the dependency information in one place.
| @@ -6,7 +6,7 @@ import React from 'react'; | |||
| import debounce from 'debounce-promise'; | |||
| import AsyncSelect from 'react-select/async'; | |||
|
|
|||
| import {Theme} from 'mattermost-redux/types/preferences'; | |||
| import {Theme} from 'mattermost-redux/selectors/entities/preferences'; | |||
There was a problem hiding this comment.
Wouldn't this come from @mattermost/types and not selectors? Same with other instances of this
There was a problem hiding this comment.
This I have copied from the mattermost-plugin-calls.
https://github.com/mattermost/mattermost-plugin-calls/blob/d4a96f4ceef5c12e55822b1d18195967dd071c59/standalone/src/theme_utils.ts#L2-L3
There was a problem hiding this comment.
Interesting, the Theme is indeed defined in mattermost-redux/selectors/entities/preferences.ts and not @mattermost/types/preferences.ts. @hmhealey Any ideas if this is intentional, and if it should be moved to @mattermost/types? And maybe or maybe not exported from selectors as well to keep backwards compatibility. Or we can view mattermost-redux as not following semver and make other projects reference @mattermost/types. Or we can just leave in selectors.
There was a problem hiding this comment.
Moving it to @mattermost/types makes sense to me. When we moved it to mattermost-redux a couple years ago for MM-46374, that had been our end goal. There's a couple weird/outdated fields in it that I wanted to get rid of, but I'd rather accept those if it helps get away from having plugins import from the web app directly
webapp/src/components/modals/channel_subscriptions/edit_channel_subscription.tsx
Outdated
Show resolved
Hide resolved
webapp/package-lock.json
Outdated
There was a problem hiding this comment.
Wondering how the changes in package-lock.json come up. Does this mean npm analyzes all of the dependencies that mattermost-webapp has? Or really just the three directories we're importing from there?
There was a problem hiding this comment.
Yeah, it does. If you open up the file, you'll see references to dependencies such as "mattermost-webapp/webapp/channels/node_modules/@hot-loader/react-dom"
There was a problem hiding this comment.
@hmhealey @Kshitij-Katiyar Do you know if we are able to make it so the channels package is not affecting the package-lock.json here? Or does this method of cloning/importing mean that we need all deps from the core webapp?
There was a problem hiding this comment.
mattermost-redux is in that package, so no, we can't. It'd need to be a separate package with its own package.json to not do that.
Unfortunately, to avoid moving around all the files in the web app, I avoided setting up a separate package for it when I moved it into the web app monorepo a few years ago, and then I did the same thing when moving it into the server monorepo because we had planned to ship a proper mattermost-redux replacement after that
Summary
Added the mattermost-webapp from the mono repo of mattermost to add the newer mattermost-redux package. Took the reference from the https://github.com/mattermost/mattermost-plugin-calls/pull/555/files.