Plugin System Upgrades


(Robin Ward) #1

I’ve been working quite a bit recently on upgrading Discourse’s plugin architecture to allow for more complex plugins such as tagger without all the megahacks that were there before.

I’ve still got a bunch to do but I figured I’d throw out some ideas here and see what plugin authors and the dev team think of them.


Problem: Inserting Content into Templates

Currently, in a plugin if you define a handlebars template with the same name as one in discourse, it will overwrite it. This is okay if the template is simple, but if you just want to add some information to an existing template it is quite onerous.

Potential Solution: Plugin Outlets

In my pluginnew branch, I have added support for the following.
In the main discourse codebase, we can identify points where we will allow templates to be injected into ours.

This can be done as simply as {{plugin-outlet "composer-open"}}, where composer-open is a string that identifies the extension point for the plugin.

Then, in a plugin, if you create a connector, which is a template that will be inserted at that plugin-outlet. You identify a connector by putting it in a directory called connectors/composer-open or whatever the name of the plugin-outlet was. So for the tagger work I’m doing, I called the file connectors/composer-open/tags.js.handlebars and put in the tag specific handlebars to be inserted into the composer when it’s open.

Another advantage to this is that multiple plugins can insert templates into the same outlet. It will just append them sequentially.


Problem: Explicitly Declaring all your javascript files is annoying

Right now you have to make a call to register_asset for every javascript file you include. This is tedious, and annoying if you ever rename or move a file.

Potential Solution: Automatically include every javascript file

I am afraid that if I include this it might break some plugins, so for now I have restricted it to .es6 files only as they are pretty new. In the latest master if you put an .es6 file in your project, it will be included automatically. I would love to hear thoughts on whether there is a good reason to NOT include all javascript by default.


Problem: How to extend classes using ES6 modules?

I am in the process of converting Discourse’s javascript code base to use ES6 modules. I am currently restricting this to 30 minutes of work a day, but it’s coming along and before we know it we’ll be using ES6 modules for everything.

One issue with these modules is the code inside them is not executed automatically. So if you are used to calling reopen or reopenClass on something to add more functionality it will not happen.

Solution: Use an initializer

If you put a file in the initializers folder that returns an Ember initializer, it will be executed when the application starts up. Just return a plain old Javascript object with a name field and a initialize function. Do your class modifications in there and everything will work nicely!


I’d love to hear thoughts on this so let me know what you think before I merge it into master.


How to create a plugin that change the layout and add new table
How are you dealing with discourse template customizations?
Add content to default template
Custom field for 'create an account'
What makes discourse.org hosting better than discoursehosting.com, if anything?
Full-page search confuses the record of the latest post you read
Adding plugin-outlets using a theme
(Benjamin Kampmann) #2

As mentioned previously, I am a big fan of making positions used by multiple plugins more explicitly extensible through explicit hooks. So thanks for working on this!

Just so that I don’t misunderstand, the way this works would be that plugins could put a template into a certain subfolder with a specific name and if such plugin-outlets are found they will be wrapped into Ember.Views and rendered there, right?

I don’t know how the performance on {{outlet}} generally is but I assume this is so fundamental that this shouldn’t be much of a bother. But if we already do use EmberViews why not make it proper and use actual ContainerViews that plugins can then extend with their own views. I like this a little more for its “separation of concerns” as the plugin developer could ship the whole view intelligence without having to patch the source views too much (as its child-views they share the same model and controller, afaik).

Instead of having a handlebar helper, just using a specific specific collection view that’d also load implicitly based on the folder name. I for once am using a collection view that is automatically rendering its children in order of system settings in my sidebar plugin.

Either way I’d like to have a global registration point for these extension points so that plugin authers could register views programatically, too. And I personally feel the CollectionView is just a more proper way of doing this.

Yeah that sound more trivial than it currently is. We have multiple plugins doing this using the HAQL patches right now and one problem we do have, is that the order plugin is unpredicted. It seems to depend on the order discourse is loading them but as you can name the folder what-ever-the-f-you-like there is no predictability whatsoever. We should come up with a solution to have a predictable loading-order of plugins, too and maybe allow admins to change the order (and maybe even hide specific views) on a per-extension-point-basis.


However for more complex plugins like the before mentioned [sidebar plugin][1] that actually needs to replace parts of the code, this approach isn’t sufficient enough. The same goes for something that digs deeper into certain functionality like the questions plugin. I do understand that both are not of high importance to the discourse core team but as an external freelancer I have clients that want changes that deep and I’d appreciate a system for those, too.


Ehm… cat plugin.js

//= require_tree .

At least that’s what we are doing on some of those projects with a lot of files. If we had an official rake create:plugin command, I’d recommend it just created that file and we are good.

The only reason I could think of to not make it happen implicitly is because of sorting order. If I’m not mistaken Ruby usually orders them alphabetically and it would be highly annoying if I had to rename my view and its file from AdsenseView into something else because it extends the GeneralAdvertisingView. But I assume that would be handled by the dependency management in .es6 anyways, or? Then for es6 that’d be fine. For the old-school JS, I’d still go for explicit (or main-file) inclusion.


The initializer Idea sounds good enough to me. Do you have an example case/plugin somewhere?

[1]: GitHub - gnunicorn/discourse-plugin-sidebar


(Robin Ward) #3

Actually plugin-outlet is a custom control that I created for our purposes in Discoruse. It doesn’t use the {{outlet}} code.

Under the scenes though, it is already implemented as a ContainerView as you suggest. Each additional plugin declared will be appended to the container view, or no container view will be inserted if no plugin matches the outlet name.

What would be the advantage of this? Maybe an example showing why just using the template name wouldn’t work? BTW, it will also use a view called views/connectors/outlet-name if outlet-name exists. So you can create your own custom view class for your inserted view.

This is a good idea. Do you have an example of a couple of plugins I could look at where order matters? It would help in isolating and making this configurable.

I was actually not thinking you could do that. Still, I think it should be on by default. I’ve enabled it for ES6 for now and that’s pretty good going forward. I’ll leave the old JS files as is (although I might have to enable it for templates since they don’t have the ES6 extension?).

Any of the current ones in our app work the same way:


(Benjamin Kampmann) #4

I didn’t realise. That sounds more than fine then. Same for the following comment about outlet-name-views.

The only clash I can think of that we had was with the (closed-source) adsense changes in combination with tagger. As well as the questions plugin modifications of the composer and tagger. Neither example can be shown on vanilla discourse.

I’d have a proposal for that though. If you look at this PR I made for post_menu-admin-lists. There is also a paragraph describing how other plugins can be part of the autocomplete. If we’d add another admin-section called “plugin-outlets” or something, containing simply the list of all plugin-outlet names, this would be easy to configure there. Then make the plugin outlet look up the order in which to load the items from the settings (for e.g. this way) – easy peasy. Would also allow the admin to disable specific UI changes of plugins by not specifying them in there (and I love that!).


(Robin Ward) #5

An admin section to configure it could work. Eventually we need plugins to be able to provide their own settings and stuff anyway, but that is a much larger and long term project.

I wonder if there’s something simpler we can do in the meantime. Ember’s initializers have an interesting approach. You name each one, and then you can specify that your initializer executes after another initializer by name.

Maybe there could just be a bit of Javascript included with a connector that says “after X” where X is another connector you want to make sure is inserted first.


(Kane York) #6

It can also be useful to say “run me before X”, but this requires building a dependency map… Eh, don’t worry about that for now.


(Robin Ward) #7

If we build it on top of Ember initializers we can use their dependency map!


(NomNuggetNom ) #8

I think this would be great, as long as there was a way to manually disable them. I suppose you could just rename the file to name.js.old or something, though.

Good stuff :slight_smile:


(Benjamin Kampmann) #9

Hey, @eviltrout,

I extended the plugin-outlets to support fall-back template data and that way can be used to wrap parts of the UI which can be overwritten. The PR is here:

https://github.com/discourse/discourse/pull/2449

With that PR and the mentioned patch to discourse, you can create the question and sidebar plugin without HAQL entirely \o/ .


(Robin Ward) #10

Awesome, I’ve just merged it in.


(Sam Saffron) #11

@eviltrout

One big issue I have is the File-litis this suffers.

If I just want to insert a single point in. I need to:

  1. Create the folder assets
  2. Create the folder javascripts
  3. Create the folder connectors
  4. Create the folder connector_name
  5. Create the file my_thing.js
  6. register it …, its just too much pain.

Instead, in plugin.rb

Option 1:

register_plugin_outlet "my-outlet", <<HBR
Some awesome template {{test}}
HBR

Option 2:

register_plugin_outlet "my-outlet", "assets/my_asset.js"

I get convention and all, but we want to reduce friction here as much as possible. I would like to support both of these perhaps in conjunction with what we have now.

Also, performance note:

Can we make sure no views are created if nobody is listening on the outlet I would actually go as far as to say that you would only create the container view if more than 1 view is registered.

I would like to make sure we can sprinkle this stuff throughout our code with close to 0 impact. Uneeded views and nested views need to go.


(Robin Ward) #12

You don’t have to register files as long as you create them with the .es6 extension (which you should be doing!)

The directories can be a bit nested but mkdir -p reduces 4 of your steps. Plus, if you are using outlets you are probably going to need assets/javascripts/discourse anyway. We should consider giving a plugin template, maybe with yeoman that scaffolds the basic stuff with some gitignores.

I am not against Ruby sugar to avoid making files but your examples do not differentiate between views and templates, both of which can have connectors.

Additionally the plugin outlet doesn’t create a container view if nothing is registered, never has :smile:


(Sam Saffron) #13

But… it create 2 views for cases where there is one user of the outlet, that is not needed. It can defer creation of a container view until there are 2 consumers.

Fair point:

register_plugin_outlet_template "test", "Some javascript" 

or

register_plugin_outlet "test", template: "Some handlebars", view: "Some javascript"

I just don’t understand what value nesting a ton of folders is giving us. its busy work that is very error prone.

For example:

Why assets/javascripts … just javacripts should be good enough, boom one directory is gone.


(Benjamin Kampmann) #14

Oh, this is so cool. At the moment implementing all kinds of minor UI/UX-Tweaks in a plugin by wrapping existing template-bits into the new {{#plugin-outlet}}{{/plugin-outlet}}-feature in our discourse branch and adding the corresponding files in the plugin. Separation of concerns for the win \o/ .


+1000 - YES PLEASE!

Though I think a rake-task would make more sense atm (as we don’t yet have yeoman in our dev environment). But yes, a template would be great.


While talking about it, one thing I do not like about the whole plugin-registry-thing in general is that it doesn’t get properly reloaded in development. So at the moment having one register_plugin_asset with the content //=require_tree . just makes it much easier for development when creating new, renaming files and folders. Without it I often found myself wondering why stuff doesn’t work simply because I forgot to change the reference and restart the dev-server. This is even worse for the after_initialize-keyword which isn’t re-evaluated when the code is reloaded on the dev-server. Leading to a constant restart-cycle when working on that part.

I agree with @eviltrout that probably everything found in assets should just be included no matter what (unless you set an include_all_assets : false-flag in your plugin.rb). And I see little problems as with es6 we have proper dependency handling now.


Though I consider the overhead comparably small (to other parts of the UI especially) this is easy to fix. @eviltrout if you want I can send a PR for that.

What I find a little more ugly is that I can’t seem to have access to whether we are wrapping an area or not at the time of creating the plugin-outlet-views since my latest addition. And though this is a virtual and rather lightweight view we are creating, if we don’t have something to render, it is useless. @eviltrout if you have any idea, how we might be able to prevent this from happening, this is nagging me a little, too .

But again, neither of them are really bloating up the memory overhead as far as I can tell.


#15

@lightyear you have worked hard and steady on this tags component. I hope it moves past plugin status and becomes a built in feature of Discourse one day :smile:


(Benjamin Kampmann) #16

Thanks, highly appreciated!

Though, to be honest, I am very happy with tagger being a plugin. I am a big fan of plug-able systems and bloat-reduction through feature deactivation. I don’t think everything should be done in the core directly to allow for flexible adoption to the specific case. As a matter of fact, I see a few things I’d rather see as plugins in core right now (like the blog-posts-feature). But as a system discourse isn’t there yet to support all these cases yet and I will continue to work on making it that way :slight_smile: .

With these changes here, however, I think tagger can easily become an highly stable, external plugin to offer this feature to whomever needs it on their discourse v1.


(Sam Saffron) #17

Not for the topic list, or posts, we need to keep view counts there as low as possible we already have 6-15ms per post render.


(Robin Ward) #18

I’ve patched it so that when there is exactly one connector, that view is inserted into the template rather than a container with one view. Also, I’ve made it so that there isn’t even a virtual view rendered when there is no block provided in the block form of the helper:

https://github.com/discourse/discourse/commit/61fb0f736bd968ceb029353f3206b19f63f19205

The main reason you have to nest so many is to play nicely with ember-rails, and specifically the asset pipeline, which currently is responsible for compiling our templates. We don’t have much control over how it creates the paths it uses.

If we created our own ember-rails for compilation or extended it, I’d be fine with just a templates/connectors/xyz/abc folder which is quite a bit shorter.

I also dislike that we have to register templates now. The es6 stuff is included automatically and that’s very productive, but handlebars templates are not. I really feel that we shouldn’t have to declare it separately. What good reason is there to have a file in assets and not include it?


(Jeff Atwood) #19

How much of this is still relevant today? Should this topic be updated or removed?


(Sam Saffron) #20

Still fairly relevant, we still use these patterns