search

mattermost / mattermost-plugin-starter-template

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

Include stubs for all hooks to get going quickly #110

Open mickmister opened 4 years ago

mickmister commented 4 years ago

Original Message

If https://github.com/mattermost/mattermost-plugin-starter-template would contain a stub implementation of all hooks, new author could use it from the get go and even if they leave the unneeded hooks, it's not a big deal.

_Issue created from a Mattermost message by @hanzei._

mickmister commented 4 years ago

This would be a great learning experience for someone to learn about the different hooks available in depth.

lieut-data commented 4 years ago

Note that this has a non-zero impact on performance if a user mistakenly leaves these stubs in place: the server optimizes away the need to notify plugins of hooks that it doesn't register.

mickmister commented 4 years ago

@lieut-data I was thinking the same. Do you have any ideas of how we can help a plugin dev new to the framework have some sort of IDE support for the hooks? Linking to the docs does our due diligence, but there are so many functions that having autocomplete like p.API, but for hooks, would be immensely helpful. I am not aware of a way to do this with Go for creating methods on a struct. Please see the discussion at https://community-daily.mattermost.com/_redirect/pl/3oyebhq9w7b9bmzmbemz5573dh for context.

lieut-data commented 4 years ago

I'm not particularly fond of the "implement the method" magic that currently exists. I'd much rather have something like a p.API.RegisterExecuteCommand(p.ExecuteCommand) (or p.Hooks.RegisterExecuteCommand(p.ExecuteCommand)).

In principle, this is possible, but it requires reworking the implementation of hooksRPCServer to support same, looking to a registry defined at run-time instead of reflecting. It's a bunch of fiddly work that might duplicate code (unless we relocated all of it to the mattermost-plugin-api and had the server depend on /that/). But at the end of the day, we get auto-completion :D

mickmister commented 4 years ago

unless we relocated all of it to the mattermost-plugin-api and had the server depend on /that/

@lieut-data That would just be another place that needs to be updated whenever a new hook is implemented :) Our docs are a bit out of date in that regard and I have a todo to update those. I suppose we could have the stubs and comment them out?

lieut-data commented 4 years ago

That would just be another place that needs to be updated whenever a new hook is implemented :)

So in my proposal, that would be the /definition/ of the hooks, and to implement same in the server, we'd bump our dependency on mattermost-plugin-api and implement the required interface. In theory, at least, no duplication required at all.

I suppose we could have the stubs and comment them out?

This is pragmatic: I like it! (Same duplication problem, of course.)