Review and improve Extension API #246

Open
tobscure opened this Issue Aug 27, 2015 · 14 comments

Projects

None yet

5 participants

@tobscure
Member

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?

@tobscure
Member
tobscure commented Sep 3, 2015

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.

@justjavac justjavac referenced this issue in justjavac/Flarum Sep 7, 2015
Open

Flarum v0.1.0 开发路线图 #3

10 of 53 tasks complete
@tobscure
Member
tobscure commented Sep 8, 2015

@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. :/

@tobscure
Member

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. 😄

@tobscure
Member

@darkspotinthecorner Probably not at this stage, but I would still be interested to see a concrete example of how this would actually look (e.g. a gist with an example component and an example extension). :)

@darkspotinthecorner

@tobscure I'll have a go, then. ;)

@tobscure tobscure added a commit that referenced this issue Oct 8, 2015
@tobscure tobscure Major refactor and improvements
- Reorganised all namespaces and class names for consistency and structure. Following PSR bylaws (Abstract prefix, Interface/Trait suffix).
  - Move models into root of Core, because writing `use Flarum\Core\Discussion` is nice. Namespace the rest by type. (Namespacing by entity was too arbitrary.)
  - Moved some non-domain stuff out of Core: Database, Formatter, Settings.
  - Renamed config table and all references to "settings" for consistency.
  - Remove Core class and add url()/isInstalled()/inDebugMode() as instance methods of Foundation\Application.
  - Cleanup, docblocking, etc.

- Improvements to HTTP architecture
  - API and forum/admin Actions are now actually all the same thing (simple PSR-7 Request handlers), renamed to Controllers.
  - Upgrade to tobscure/json-api 0.2 branch.
  - Where possible, moved generic functionality to tobscure/json-api (e.g. pagination links). I'm quite happy with the backend balance now re: #262

- Improvements to other architecture
  - Use Illuminate's Auth\Access\Gate interface/implementation instead of our old Locked trait. We still use events to actually determine the permissions though. Our Policy classes are actually glorified event subscribers.
  - Extract model validation into Core\Validator classes.
  - Make post visibility permission stuff much more efficient and DRY.

- Renamed Flarum\Event classes for consistency. ref #246
  - `Configure` prefix for events dedicated to configuring an object.
  - `Get` prefix for events whose listeners should return something.
  - `Prepare` prefix when a variable is passed by reference so it can be modified.
  - `Scope` prefix when a query builder is passed.

- Miscellaneous improvements/bug-fixes. I'm easily distracted!
  - Increase default height of post composer.
  - Improve post stream redraw flickering in Safari by keying loading post placeholders with their IDs. ref #451
  - Use a PHP JavaScript minification library for minifying TextFormatter's JavaScript, instead of ClosureCompilerService (can't rely on external service!)
  - Use UrlGenerator properly in various places. closes #123
  - Make Api\Client return Response object. closes #128
  - Allow extensions to specify custom icon images.
  - Allow external API/admin URLs to be optionally specified in config.php. If the value or "url" is an array, we look for the corresponding path inside. Otherwise, we append the path to the base URL, using the corresponding value in "paths" if present. closes #244
dd67291
@tobscure tobscure added a commit that referenced this issue Oct 13, 2015
@tobscure tobscure Initialise component state in init() instead of constructor
This allows component state to be overridden via monkey-patch. ref #246
33dd5ff
@tobscure
Member

Alright @YoruNoHikage, all components now initialise their state in init() instead of the constructor :)

@YoruNoHikage

Ok, thanks, I hope there would be no problem now. :)

@sijad
Contributor
sijad commented Feb 9, 2016

can we see monkey patching in back-end too?

@franzliedke
Member

Monkey patching works very well with JS because of its prototype-based inheritance model (and the very loose typing).

But unless you have some example of an extensibility use case that you cannot solve without it, we will probably keep the purely OO based, event-centric approach we currently use in the backend...

@franzliedke franzliedke modified the milestone: 0.1.0 Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment