Review and improve Extension API

[tobyzerner]

I would love to get some feedback on Flarum's Extension API. If you haven't seen much of it, check out the documentation, as well as the source code for all of Flarum's packaged extensions.

Scoping

Currently we don't distinguish very well between what's part of the public API and what's not – and if we want to do SemVer, we need to do this much better. Given how the API is set up, how can we effectively do this? Some ideas:

• Prefix all private JavaScript variables and methods with _. Make sure docblocks reflect that too.
• Even if something is public, we may not want to guarantee that we won't break it. Should we explicitly use the @api docblock tag to denote our public API?
• On the backend, we need to more methods private instead of protected.
• Should we make any classes/methods final?

Monkey Patching

Extending the JavaScript application uses monkey patching as its primary mechanism. I chose this because it was the simplest, most straightforward option – however, I'm wondering about a few potential pitfalls:

• There's no way for extensions to specify the priority of their monkey patch. This is not a huge problem because most monkey patching is done to add items to an ItemList, which allows for prioritization.
• Functions directly exported by modules cannot be monkey patched – i.e. helpers and some utils. Workaround is to make helpers into components, and make utils into objects containing functions.
• Class constructors cannot be reliably monkey patched because sometimes the constructor property is used to access a static method (e.g. this.constructor.whatever()). We could work around this by making our monkeypatch methods copy over the original object's properties.

The obvious alternative is to have an event-based API, like on the backend. This would entail firing events in all methods that were previously subject to monkey-patching, and then changing all instances of extend and override into event listener registrations. (This would help to better define the scope of the API too.) While a big change for this stage of development, it's certainly not too late. I would just like to get some second opinions.

Naming

I think I want to tighten up the naming of Flarum\Event classes, as they're a bit inconsistent at the moment. That and elsewhere, are there any names that you find confusing, and do you have any suggestions for replacements?

General Feedback

If you have experience with public APIs, can you tell us: What could be better about Flarum's API? What parts are good, what parts are bad?

[tobyzerner]

Some random ideas I've come up with while developing a Reports extension:

PHP

• Use our own dispatcher abstraction
• Change Action $include ['relationship' ⇒ true/false] to $includeDefault and $includeOptional. Update BuildApiAction event too.
• Make more properties and methods private
• Clean up Events: rename, split BuildClientView, document
• Throw exceptions in Events if incorrect namespacing
• Rename Extension::listen to subscribe?
• Replace app() with flarum() probably (prevent conflicts with Laravel etc)

JavaScript

• Use app.store.models.discussions.prototype.whatever = Model.attribute('whatever'); instead of Discussion.prototype.whatever = ... ?
• Make more properties and methods private
• Experiment with ability to monkey patch constructor using extend or override
• Return relationship objects is JS Model instead of just plain arrays?
  • methods like detach/attach/sync to save relationships properly via JSON-API
• Make most helpers into components

[YoruNoHikage]

I'm currently trying to develop a small extension, so here's my (little) feedback :

I just encounter the monkey patch constructor problem, I wanted to add some props to the TextEditor component. I think switching to an event-based api (or action based like rackt/redux ) could improve code decoupling.
Also, I just read this thing http://stackoverflow.com/a/28648214 and I think this could be useful when it comes to inheritance with static methods.

EDIT : I'm also encountering another trouble in PHP part :
I want to save some data when the Post is saved like this :

Post::saved(function($post) {
    // do some stuff
});

but since the Post is a CommentPost, it doesn't fire the event... See http://laravel.io/forum/06-08-2015-events-not-firing-on-inherited-model-how-can-i-fix-this or maybe I'm doing it wrong.

[tobyzerner]

@YoruNoHikage Thanks for the feedback!

Regarding the saved event, I think it's fine to go ahead and use the CommentPost class for that. That's what we're doing currently in the Akismet extension. In fact, perhaps it might be cleaner to do something like this, so that we don't have to worry about using the right class at all:

$post::saved(function ($post) {
    // do some stuff
});

I'm actually hesitant to switch away from monkey patching now, because it will be a big time loss. I think we can make the monkey patching work, especially since it's effectively just a different form of event handling. I've added an init method to the Component base class that is called in the constructor, so we can monkey-patch that instead of the constructor :)

[YoruNoHikage]

Oh right, thanks for the tip, I totally missed the fact that I could use $post. And thanks for the init function, I'll implement it right away ! If I see something that could be changed for better, I'll post some new feedback.

[YoruNoHikage]

Something wrong with the init function, see :

class Component {
    constructor() {
        // define some properties
        this.init();
    }
}

class SubComponent extends Component {
    constructor(...args) {
        super(...args);
        // define some new properties
    }
}

And here's the problem when monkeypatching SubComponent like this :

extend(SubComponent.prototype, 'init', function() {
    // Unable to access to the new properties defined in SubComponent
});

To see a concrete case, just look at EditPostComposer/ComposerBody, no way to tweak this.editor. :/

[tobyzerner]

Interesting. I can think of two possible solutions:

1. Move the contents of all sub-component constructors into the init method, so it will be run before any monkey patches.
2. Forget about monkey-patching constructors, and just monkey-patch view instead with conditionals, e.g.:

if (!this.hasBeenMonkeyPatched) {
  applyMonkeyPatch(this);
  this.hasBeenMonkeyPatched = true;
}

I think number 1 is the way to go. Thoughts?

[YoruNoHikage]

Not really. First solution requires to be very strict and means a bit of refactoring but that could be good.
The second, when implemented in core could work but feels hacky (and maybe performance losing ?). Also, It can be done in extension but I don't recommend it since extensions could conflict on variable naming.

[darkspotinthecorner]

I don't want to be disruptive here, but would you be open to refactoring?

Prototypal OO has a lot to offer here that may writing extensions a whole lot easier.

Maybe I'll be just doing this myself, just to test how it works out. Not yet sure if this would be a good or bad idea. ;)
If it works out, I'll sub a PR. 😄