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

Update links to mattermost-redux source code after migration #1390

Closed

Conversation

wetneb
Copy link
Contributor

@wetneb wetneb commented Jul 8, 2024

The current docs point to
https://github.com/mattermost/mattermost-redux, which has migrated to https://github.com/mattermost/mattermost-webapp, which has itself migrated to https://github.com/mattermost/mattermost. So it's not very convienient to access the current version of the source code.

This PR updates those links accordingly, so that the reader doesn't have to jump through so many hoops.

I removed the line numbers for links which pointed to the master branch instead of a specific commit as they get outdated as the source code changes. The reader can easily find the function using GitHub's menu or via text search. One could also convert those to permalinks (linking to the current commit), which has itself downsides as the reader will eventually be pointed to outdated source code.

The current docs point to
https://github.com/mattermost/mattermost-redux, which has migrated to
https://github.com/mattermost/mattermost-webapp, which has itself
migrated to https://github.com/mattermost/mattermost. So it's not very
convienient to access the current version of the source code.

This PR updates those links accordingly, so that the reader doesn't have
to jump through so many hoops.

I removed the line numbers for links which pointed to the master branch
instead of a specific commit as they get outdated as the source code
changes. The reader can easily find the function using GitHub's menu
or via text search. One could also convert those to permalinks (linking
to the current commit), which has itself downsides as the reader will
eventually be pointed to outdated source code.
@mattermost-build
Copy link
Contributor

Hello @wetneb,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@cwarnermm cwarnermm self-requested a review July 8, 2024 15:59
@cwarnermm cwarnermm added the 2: Editor Review Requires review by an editor label Jul 8, 2024
@cwarnermm cwarnermm requested a review from hmhealey July 9, 2024 14:27
@cwarnermm cwarnermm added 1: Dev Review Requires review by a core commiter preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories labels Jul 9, 2024
@cwarnermm
Copy link
Member

@hmhealey - Can I get your help to confirm that the proposed paths and link updates align with current repository architecture and release workflows, please?

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.

Thanks for the PR, @wetneb!

As you've found, that library is in a weird spot for plugin use because we stopped shipping it when it was moved into the mattermost-webapp repo. The previous links pointed to the latest published version of the source which still works for most things. I don't work on plugins very often, so I'll see what the recommendation is from people who work with plugins more often.

@cwarnermm I'll reach out to ABC team on that since I think they're the ones who own the plugin framework

@cwarnermm
Copy link
Member

Any update on this from the team, @hmhealey?

@hmhealey
Copy link
Member

I asked around a bit to see who to talk to, but I didn't have a chance to actually discuss it with anyone really. I started a thread in the Integrations channel on Community to see what others are thinking.

Personally, I'm still leaning towards leaving these docs as-is at the moment, but I've always preferred not recommending the monorepo version of mattermost-redux, so I want to see how people who actually use it feel about it

@wetneb
Copy link
Contributor Author

wetneb commented Jul 29, 2024

As you've found, that library is in a weird spot for plugin use because we stopped shipping it when it was moved into the mattermost-webapp repo.

I didn't notice that at all actually. If you stopped shipping mattermost-redux since this migration, is it because this library is meant to be replaced by something else? In that case, could the docs be updated to point to that instead?

The previous links pointed to the latest published version of the source which still works for most things.

What about the cases for which it does not work? What are we supposed to resort to?

As a plugin developer, I expect that the recommended library to interact with Mattermost's state is actively maintained, reflecting the state of the latest stable version of Mattermost, and therefore that the repository that hosts its source code is not archived.

@cwarnermm
Copy link
Member

@hmhealey - Are you open to answering the questions above?

@hmhealey
Copy link
Member

hmhealey commented Aug 2, 2024

As you've found, that library is in a weird spot for plugin use because we stopped shipping it when it was moved into the mattermost-webapp repo.

I didn't notice that at all actually. If you stopped shipping mattermost-redux since this migration, is it because this library is meant to be replaced by something else? In that case, could the docs be updated to point to that instead?

That was the original plan, but unfortunately, we never got to making a full replacement for the Redux actions and the utility functions which are actually used from it.

The previous links pointed to the latest published version of the source which still works for most things.

What about the cases for which it does not work? What are we supposed to resort to?

The vast majority of functions that we'd expect most plugin devs to use should keep working. For example, all of the examples in the docs page you updated are basically unchanged outside of things like improved type definitions and code style changes.

Because mattermost-redux is compiled into the plugin, the main risk of breakage would be if the shape of the Redux state changes or the types of the actions changes, but we avoid doing both of those for exactly that reason. Even after all this time, I don't know if anything in the library is actually broken, but if it is, it's likely either because it's logic that was specific to the apps which ended up in mattermost-redux by mistake, so I wouldn't think people would even want to use. A big part of the reason that we stopped shipping mattermost-redux is because the library included too much internal logic for us to maintain and ensure compatibility for.

The other option would be to do like many of our internally maintained plugins and just not use Redux at all. We publish the API client separately at https://www.npmjs.com/package/@mattermost/client, and it's updated for each minor release. Many of the actions in mattermost-redux are just wrappers for its methods, so you may need to fetch some data yourself, but you won't need to worry about all the cruft that even a new version of mattermost-redux would include.

As a plugin developer, I expect that the recommended library to interact with Mattermost's state is actively maintained, reflecting the state of the latest stable version of Mattermost, and therefore that the repository that hosts its source code is not archived.

Yeah, I totally understand that, and I very much agree with that. Unfortunately, the team that originally maintained the plugin infrastructure specifically has ended up shifting more to maintaining and working on new plugins rather than being able to work full time on it, so some parts like mattermost-redux have been mostly left to the wayside because they're not seen as essential to letting people make plugins. We've done a bit of work on replacing and on resuming publishing it, but we haven't been able to prioritize it yet because the current version is solidly "good enough" for most purposes

@wetneb
Copy link
Contributor Author

wetneb commented Aug 2, 2024

Thank you so much for the extremely thorough explanation, which looks like a very good basis for documentation!
Then I agree that updating the links doesn't make sense.

@wetneb wetneb closed this Aug 2, 2024
@hmhealey
Copy link
Member

hmhealey commented Aug 6, 2024

Thanks for your understanding on this, and thanks for the work you put in on it as well. Even though we're leaving it unchanged, it's generated some good discussion both here and on our community server as well. Hopefully we can use that to help get a bit more time to work on improving this 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1: Dev Review Requires review by a core commiter 2: Editor Review Requires review by an editor Contributor preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants