MarionetteLabs / Sesame.js

Experimental Framework
0 stars 2 forks source link

Object Model Requirements #6

Closed jamiebuilds closed 8 years ago

jamiebuilds commented 9 years ago

Creating an issue to start a discussion about how the base object model should work.

In Marionette there is:


I want to propose removing this.options, getOptions, and mergeOptions in favor of the React style propTypes.

import {Class, Model, PropTypes} from 'sesame';

export default Class.extend({
  props: {
    user: PropTypes.instanceOf(Model).isRequired
  },
  initialize() {
    console.log(this.user.isLoggedIn()); // true
  }
});

This can auto-merge properties, check that you are passing the correct properties, and there can be sesame.{development,production}.js builds that only check the type in development so that you don't have any performance penalty in production.


I also want to suggest that [un]bindEntityEvents be moved to utilities that do not live on the base object model (if they are needed at all).


Argument for removing [un]bindEntityEvents entirely (including modelEvents, collectionEvents, etc):

When you're working with views that have multiple models/collections, it's generally better to use named models/collections (this.user instead of this.zone). We've now made it a rule on our app that everything should always be named, and it has improved clarity dramatically.

This also means never using modelEvents and collectionEvents. However, TBH I don't miss them at all.

In fact, it might even be better without them, because we're using the same API we use all over the place rather than a couple of exceptions.

View.extend({
  initialize(options) {
    this.user = options.user;
    this.zone = options.zone;

    this.listenTo(this.user, {
      login: this.onUserLogin,
      logout: this.onUserLogout
    });

    this.listenTo(this.zone, {
      'change:name': this.onZoneChangeName
    });
  },
  // ...
});

I also want to propose changing triggerMethod into an internal utility for doing this:

function addHook(target, method, event, params) {
  target[method](params);
  if (target._events[event]) { // avoid penalty of ...params if possible.
    target.trigger(event, target, ...params);
  }
}

export default Class.extend({
  action() {
    addHook(this, 'onAction', 'action', [additional, params, here]);
  }
})

or just:

export default Class.extend({
  action() {
    this.onAction(additional, params, here);
    this.trigger('action', this, additional, params, here);
  }
})

This is going to improve performance, rather than the weird string manipulation that there is now.

Also, custom triggerMethod's have had a tendency of being a source of confusion in our app.

jamiebuilds commented 8 years ago

Clearing out old issues, closing for inactivity