jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.38k forks source link

Backbone and ES6 Classes #3560

Open benmccormick opened 9 years ago

benmccormick commented 9 years ago

With the final changes to the ES6 class spec (details here), it's no longer possible to use ES6 classes with Backbone without making significant compromises in terms of syntax. I've written a full description of the situation here (make sure to click through to the comments at the bottom for an additional mitigating option), but essentially there is no way to add properties to an instance of a subclass prior to the subclasses parents constructor being run.

So this:

class DocumentRow extends Backbone.View {

    constructor() {
        this.tagName =  "li";
        this.className = "document-row";
        this.events = {
            "click .icon":          "open",
            "click .button.edit":   "openEditDialog",
            "click .button.delete": "destroy"
        };
        super();
    }

    initialize() {
        this.listenTo(this.model, "change", this.render);
    }

    render() {
        //...
    }
}

is no longer valid in the final ES6 spec. Instead you effectively have 3 (not very appealing) options if you want to try to make this work:

Attach all properties as functions

Backbone allows this, but it feels dumb to write something like this:

class DocumentRow extends Backbone.View {

    tagName() { return "li"; }

    className() { return "document-row";}

    events() {
        return {
            "click .icon":          "open",
            "click .button.edit":   "openEditDialog",
            "click .button.delete": "destroy"
        };
    }

    initialize() {
        this.listenTo(this.model, "change", this.render);
    }

    render() {
        //...
    }
}

compared to the current extends syntax

Run the constructor twice

I don't view this as a real option due to the issues it would cause running initialize a second time with different cids, etc.

Pass all properties as default options to the superclass constructor

This was suggested by a commenter on my blog and is probably the most practical current option. It looks something like this:

class MyView extends Backbone.View {
  constructor(options) {
    _.defaults(options, {
      // These options are assigned to the instance by Backbone
      tagName: 'li',
      className: 'document-row',
      events: {
        "click .icon": "open",
        "click .button.edit": "openEditDialog",
        "click .button.delete": "destroy"
      },
      // This option I'll have to assign to the instance myself
      foo: 'bar'
    });

    super(options);

    this.foo = options.foo;
  }
}

Since all of these current options involve clear compromises relative to the current Backbone extends syntax, it would be wonderful if a better solution could be developed. I'm not totally sure what this should look like, but one idea that came to mind while I did the writeup for my blog was the addition of a "properties" function that would output a hash of properties. The constructor could then run that function and add them to the instance prior to the other processing done by the constructor.

akre54 commented 9 years ago

Yeah this is definitely a bummer. Thanks for doing the legwork.

I guess moral of the story is don't use ES6 classes with Backbone, at least until static property support lands. Of the fallback options you proposed my preferred solution is defining the strings / objects as return values. A key part of Backbone's API design is in these prototype-shared strings and objects, and it would dirty up the API to require devs to assign each property to the instance in the constructor (not to mention being memory wasteful).

Aside from consistency, is there any reason to use the class keyword with Backbone over extend?

jridgewell commented 9 years ago

Great blog post. I'd been wonder how ES6 and Backbone classes would play together. As for you solutions:

  1. Attach all properties as functions: I'm not super opposed to this. It's not as clean as setting the object directly on the prototype, but I've seen a ton of code trip up on mutating prototype objects. This way is immune, which is why I think ES6 chose not to include class properties.
  2. Pass all properties as default options: Isn't this how you'd do something in a more classical language? I feel like this is a even less clean solution than the above.
  3. Run the constructor twice: Ick.

I guess moral of the story is don't use ES6 classes with Backbone, at least until static property support lands.

Even class properties come after the super() call. :disappointed:

benmccormick commented 9 years ago

Aside from consistency, is there any reason to use the class keyword with Backbone over extend?

I addressed this in the blog post. Practically? No. In theory it would allow Backbone in the long term to reduce code and additional concepts, but realistically its going to be at least a few years before ES6 classes are widely supported on all relevant browsers without transpiling, and the code reduction would be next to nothing.

But don't underrate the consistency aspect. If this becomes "the way" of doing Object Oriented programming in JavaScript (seems likely given the standardization on this from Ember/Angular/React/Typescript/Aurelia etc), Backbone not using it will be an added learning curve for the library relative to other options. Especially for Junior developers. I'm not sure that necessarily merits a change. But it's not just for pedantic "hobgoblin of small minds" consistency.

lukeasrodgers commented 9 years ago

I agree with @akre54 and @jridgewell that the "attach all properties as functions" approach is probably the best of the proposed options. FWIW, I remember that when I was originally learning backbone as a relative js newcomer, I was a bit confused by these "static" properties and how they should be used.

A commented 9 years ago

ES7 will have correct class properties, I guess https://gist.github.com/jeffmo/054df782c05639da2adb

benmccormick commented 9 years ago

The ES7 proposal is just that, a very early community driven proposal. Not at all clear it will actually ever be part of an official spec. Current implementations cause properties to be added to the instance AFTER the constructor runs, so it doesn't help with Backbone. (see jridgewell's link above or try it yourself with Babel 5.0.0)

akre54 commented 9 years ago

@jridgewell I was referring to this part of @benmccormick's post:

React Developers have noted the same issues with property initializers that Backbone users encounter. As part of version 0.13 of React, they're supporting a special property initialization syntax for classes, which may eventually be standardized. There's more info on that in this ESDiscuss thread. This standard is still being worked out but an experimental support version is available in Babel 5.0.0. Unfortunately that version defines class properties as being instantiated after the superclass constructor is run, so this doesn't solve Backbone's issues here.

See for example wycats' js-decorators strawman or the original (superseded) harmony classes proposal.

I might suggest that we use getters with class properties:

class Row extends Backbone.View {
  get tagName() { return 'li'; }
}

As an absolute last resort, we could check for instance or static props with a helper a la _.result:

_.instOrStaticVar = function(instance, property) {
  if (instance == null) return void 0;
  var value = instance[property] || instance.constructor[property];
  return _.isFunction(value) ? value.call(instance) : value;
}
jridgewell commented 9 years ago

Yup, but:

Unfortunately that version defines class properties as being instantiated after the superclass constructor is run, so this doesn't solve Backbone's issues here.

So, ES5'd:

// ES6
class View extends Backbone.View {
  tagName = 'li';

  constructor() {
    // Do anything that doesn't touch `this`
    super();
    // Do anything that touches `this`
  }
}

// ES5
function View() {
  // Do anything that doesn't touch `this`
  Backbone.View.apply(this, arguments);

  // Add class properties
  this.tagName = 'li';

  // Do anything that touches `this`
}
View.prototype = _.create(Backbone.View.prototype, {
  constructor: View
});

Our element would still be constructed before we got a change to set the instance variable.

See for example wycats' js-decorators strawman...

Can you explain how the decorators would apply?

I might suggest that we use getters with class properties:

:+1:. I see that as the same boat as attach all properties as functions. Not as clean as what we currently have, but perfectly acceptable and mutation proof.

As an absolute last resort, we could check for instance or static props with a helper a la _.result:

That could be interesting...

jamiebuilds commented 9 years ago

You could do:

class MyView extends Backbone.View {
  constructor() {
    super({ tagName: 'h1' });
    this.el.textContent = 'Hello World';
  }
}
jridgewell commented 9 years ago

@thejameskyle That's the Pass all properties as default options to the superclass constructor option. :stuck_out_tongue:

milesj commented 9 years ago

Instead of relying on super() to setup the class, you could simply have an init() function or something.

class DocumentRow extends Backbone.View {

    constructor() {
        super();
        this.tagName =  "li";
        this.className = "document-row";
        this.events = {
            "click .icon":          "open",
            "click .button.edit":   "openEditDialog",
            "click .button.delete": "destroy"
        };
        this.init();
    }

    initialize() {
        this.listenTo(this.model, "change", this.render);
    }

    render() {
        //...
    }
}
benmccormick commented 9 years ago

@milesj hmm? That will error out immediately with the final ES6 class spec

In a derived class, you must call super() before you can use this

Even if it did work you're never actually calling the Backbone constructor and will not get its initialization code.

See this link from my first post: http://www.2ality.com/2015/02/es6-classes-final.html

jridgewell commented 9 years ago

@milesj: The thing is, you have to call super() before setting this.tagName or the like. And, since we ensure an element in the View's constructor, we've already created an element before we'll ever set this.tagName.

jamiebuilds commented 9 years ago

@milesj that's still not allowed when you are subclassing.

@jridgewell Oh sorry, I missed that. It does seem like the most natural option. I spoke to jeffmo and sebmck about this.

To give you guys some backstory, the reasoning is because in order to support extending native types (i.e. Array) this isn't determined until you call the super() method. Otherwise you run into initialization issue in the DOM (and presumably other places).

milesj commented 9 years ago

@jridgewell @thejameskyle Then simply call super() first (updated example). I really don't see the issue here as I've done the same thing in my ES6 classes. Just move the views constructor logic to the init() method.

jridgewell commented 9 years ago

That's a lot of very expensive code to run twice.

benmccormick commented 9 years ago

@milesj did you read the original blog post? Running super first means the properties aren't processed. See here for a full in depth explanation: http://benmccormick.org/2015/04/07/es6-classes-and-backbone-js/

milesj commented 9 years ago

Yes, I've read it, and I'm still curious why this is not a solution. Everyone keeps talking about the views constructor needing to be called, but that isn't necessarily the case. Why isn't something like the following not a solution (albeit a bit contrived)?

var View = Backbone.View = function(options) {
    this.cid = _.uniqueId('view');
    // extend()ing options is no longer needed if properties are set directly
};

View.prototype.setup = function() {
    this._ensureElement();
    this.initialize.call(this, arguments);
};

class DocumentRow extends Backbone.View {
    constructor() {
        super();
        this.tagName =  "li";
        this.className = "document-row";
        this.events = {
            "click .icon":          "open",
            "click .button.edit":   "openEditDialog",
            "click .button.delete": "destroy"
        };
        this.setup(...arguments);
    }
}

I'm guessing because of backwards compatibility with non-ES6?

jridgewell commented 9 years ago

Then the default View class wouldn't work since the constructor never calls #setup. And, forcing a subclass call anything other than super() is going to be super annoying.

milesj commented 9 years ago

That's an issue that all ES6 classes have to deal with, not just Backbone. I personally solved it by using the Babel ES7 class properties spec.

jamiebuilds commented 9 years ago

@milesj As stated before ES7 class properties do not solve this issue as they aren't instantiated until the end of the constructor.

I spoke to jeffmo and sebmck about doing this:

class Root {
  rootProp = 'root';
  constructor() {
    console.log('Root', this.rootProp);
    console.log('Root', this.derivedProp);
  }
}

class Derived extends Root {
  derivedProp = 'derived';
  constructor() {
    super();
    console.log('Derived', this.rootProp);
    console.log('Derived', this.derivedProp);
  }
}

Desugaring to:

function Root() {
  this.rootProp = 'root';
  console.log('Root', this.rootProp);
  console.log('Root', this.derivedProp);
}

function Derived() {
  super();
  this.derivedProp = 'derived';
  console.log('Derived', this.rootProp);
  console.log('Derived', this.derivedProp);
}

But that still doesn't fix the issue here and leads to inconsistency:

new Derived();
// >> 'Root' 'root'
// >> 'Root' undefined
// >> 'Derived' 'root'
// >> 'Derived' 'derived'
jridgewell commented 9 years ago

That's an issue that all ES6 classes have to deal with, not just Backbone.

Hm?

I personally solved it by using the Babel ES7 class properties spec.

You're gonna have a lot of DIV elements with no classNames. See the last point of https://github.com/jashkenas/backbone/issues/3560#issuecomment-90739676, https://github.com/jashkenas/backbone/issues/3560#issuecomment-91601515 and https://github.com/jashkenas/backbone/issues/3560#issuecomment-98827719.

milesj commented 9 years ago

I see. In that case, I'd suggest going with the "Pass all properties as default options to the superclass constructor" option, or the last line about creating a "properties" method (which doesn't touch the constructor).

class DocumentRow extends Backbone.View {
    loadProperties() {
        return {
            tagName: 'li',
            className: 'document-row',
            events: {
                "click .icon": "open",
                "click .button.edit": "openEditDialog",
                "click .button.delete": "destroy"
            },
            foo: 'bar'
        };
    }
}

// Contrived example
var View = Backbone.View = function(options) {
    this.cid = _.uniqueId('view');
    options || (options = {});
    _.extend(this, this.loadProperties(), _.pick(options, viewOptions));
    this._ensureElement();
    this.initialize.apply(this, arguments);
};

I did something similar in Toolkit, which can be seen here: https://github.com/titon/toolkit/issues/107

gotofritz commented 9 years ago

Hi.

If I understand correctly the discussion here - the Backbone developers are discussing workarounds and best practice, but have no intention of actually making changes to the BB core to deal with this issue? (I'm not suggesting they should, nor would I have any idea what those changes could be). In other words, is the suggestion to use either all properties as functions or getters the final word on the topic? Thanks.

akre54 commented 9 years ago

@gotofritz We're discussing workarounds because ES6's solution of forcing all properties to live on instances doesn't scale. Backbone's class system is doing the right thing here.

There's some discussion about adding static prototype properties to ES7 classes but so far nothing concrete. In the meantime I'd say stick with Backbone's extend.

gotofritz commented 9 years ago

Thanks. I'll try ES6 classes for a little longer... :-)

For the benefit of anyone else stumbling upon this, in practice I find the "Pass all properties as default options to the superclass constructor" better - for example, our app has dynamic (localized) routes that need to be passed in at instantiation time, and having a routes() method just doesn't work. Whereas the following does

class Router extends Backbone.Router {

 constructor (localizedRoutes) {
    _.defaults(localizedRoutes, {
        "nonLocalizedRouteA/": "routeA"
        "*actions": "defaultRoute"
     });
 super({ routes: localizedRoutes });
}
raffomania commented 9 years ago

I've just had a look at this, and I think that both workarounds don't work for the idAttribute property that a Model has. A method won't work as Backbone uses model.idAttribute to access the property; And the model constructor doesn't seem to support adding properties as options altogether.

jridgewell commented 9 years ago

I think that both workarounds don't work for the idAttribute property

Excellent catch, I'll work on a PR addressing this. In the meantime, you can use getter notation to supply custom idAttribute (and cidPrefix):

class Model extends Backbone.Model {
  get idAttribute() {
    return '_id';
  }

  get cidPrefix() {
    return '__c';
  }
}
jridgewell commented 9 years ago

A method won't work as Backbone uses model.idAttribute to access the property

get idAttribute() { return '_id'; } is a getter method, which is accessed just like a normal property. this.idAttribute === '_id';.

whawker commented 9 years ago

This is beginning to sound like a major rewrite is required. Backbone v2 perhaps?

jridgewell commented 9 years ago

This is beginning to sound like a major rewrite is required. Backbone v2 perhaps?

Not at all, we already support ES6 subclassing (with the exception of Models). I think it'd be interesting if someone explored @akre54's static property suggestion, but even that isn't necessary with the two solutions in the original post.

raffomania commented 9 years ago

@jridgewell, many thanks for the quick solution!

benmccormick commented 9 years ago

Decorators were mentioned above in this thread (specifically Yehuda Katz's proposal), and it was unresolved whether that would solve this issue.

I was just playing around with them as proposed, and you can write a decorator like this:

function props(value) {
    return function decorator(target) {
        _.extend(target.prototype, value);
    }
}

and then the example we've been using can be written like this


@props({
      tagName: 'li',
      className: 'document-row',
      events: {
        "click .icon": "open",
        "click .button.edit": "openEditDialog",
        "click .button.delete": "destroy"
      }
    })
class DocumentRow extends Backbone.View {

    initialize() {
        this.listenTo(this.model, "change", this.render);
    }

    render() {
        //...
    }
}

This seems to work just fine for me. The decorator is applied to the class prior to the class constructor being executed. This is just a declarative version of saying

class DocumentRow extends Backbone.View {

    initialize() {
        this.listenTo(this.model, "change", this.render);
    }

    render() {
        //...
    }
}
_.extend(DocumentRow.prototype, {
      tagName: 'li',
      className: 'document-row',
      events: {
        "click .icon": "open",
        "click .button.edit": "openEditDialog",
        "click .button.delete": "destroy"
      }
})

Actually, I haven't tested it, but you could probably make the entire backbone extend function the decorator if you wanted both static and prototype props.

Unfortunately this is just a proposal for now, but Babel supports it behind an experimental flag, so if people are feeling adventurous, it's a possible solution here.

andrewrota commented 9 years ago

@benmccormick, the decorator technique works well for me. Other than this just being a proposal for now, are there other concerns about going with this approach?

benmccormick commented 9 years ago

@andrewrota I'm literally writing a blog post following up on this stuff right now (was reading through this thread when you commented). That is a big "other than", but I don't personally see any. I actually think we can do better than what I described above though, and create some nice new interfaces for Backbone with decorators.

See this gist from @StevenLangbroek that got me thinking about this originally: https://gist.github.com/StevenLangbroek/6bd28d8201839434b843

benmccormick commented 9 years ago

Here's a preview of the followup post I'm putting up: http://benmccormick.org/2015/07/06/backbone-and-es6-classes-revisited/ Updated with permanent link now

It'll move to a permanent url early this week sometime. But the basic summary from this thread and what I've learned is:

There are 3 approaches to making Backbone properties work with the current ES6 classes spec (the first 2 need #3684 to be considered fully supported):

  1. Pass all properties to the super in the constructor
  2. Treat all properties as methods
  3. Add properties directly to the prototype after a class has been declared

I still see all of these as limiting expressiveness to some extent or another. But I think the problem will more or less be solved if decorators become an official specification. With decorators there are 2 more options.

  1. Add a props decorator that takes the props at the top of the class and adds them to the prototype
  2. Create several special purpose decorators that allow for a more expressive/fine-grained interface.

I don't think any of these solutions require any additional modifications to Backbone other than #3684, but there would be an interesting role for a backbone-decorator library if/when decorators become standardized.

Would love any feedback on the post before I publish it on Monday/Tuesday.

StevenLangbroek commented 9 years ago

@benmccormick I figured decorators are evaluated before any construction happens, thanks for the correction. I'll update the gist in a bit. also: thanks a million for the mention in the blog post :) ping me on twitter when you publish it? :+1:

We could use the same syntax for modelEvents and collectionEvents in Marionette, just not for triggers. Those could be exposed through a class decorator (like tagName and template in your blog post), but I was thinking: can't we use static properties for this? Or does that not work in Backbone?

I understand decorators are still stage 0, but I think they'll be a great upgrade in the way we write Backbone apps, especially the method decorators over an events hash, it's the sort of programming style that makes me prefer gulp over grunt too.

benmccormick commented 9 years ago

@StevenLangbroek see above for discussion about static properties.

The syntax as currently specced creates a local property on each instance rather than adding to the prototype. Those properties are added after the super constructor is run.

andrewrota commented 9 years ago

@benmccormick, the post looks good and I think does a good job at explaining the trade offs with each of the options. At this point, I really like the special purpose decorators approach, and it seems to be the best approach assuming decorators make it into the spec.

au-phiware commented 9 years ago

Should @benmccormick's decorator call _#extend with the constructor not the prototype and then @akre54's _.instOrStaticVar method is used in place of _#result? I realise that would be a breaking change, but it seems cleaner that way IMHO. Like @akre54 pointed out properties defined that way are prototype-shared strings and objects (i.e. shared across all instances) therefore they should be accessed via the class, right?

just-boris commented 9 years ago

I am going further and make to work class properties as the way we need. Class properties can be annotated too, and we can create special decorator, which attaches decorated property to prototype.

class TodoView extends Backbone.View {
  @protoprop
  static tagName = 'li';
}

function protoprop(target, name, descriptor) {
  target.prototype[name] = descriptor.initializer()
}

See Babel REPL with example. It is relied on experimantal things, but works.

benmccormick commented 9 years ago

@just-boris as discussed in my blog comments, the behavior you're seeing there is an implementation detail of Babel's handling of the class properties and decorators specs. Its not defined behavior in any proposal right now. If you want to do things that way you'll want to make issues here and/or here to make decorators on class properties a standardized behavior. Otherwise what you're doing could (and probably will) break at any time.

just-boris commented 9 years ago

@benmccormick wycats/javascript-decorators already has extra definition regarding property initializers.

The main concern there that property initializers is a usual descriptor, as well as class methods, so decorators can wrap them as well. I don't see reasons to worry, while spec in that section remains unchanged

benmccormick commented 9 years ago

Ah very cool, I hadn't seen that. Thanks for pointing that out.

On Mon, Sep 21, 2015 at 11:29 AM, Boris Serdiuk notifications@github.com wrote:

@benmccormick https://github.com/benmccormick https://github.com/wycats/javascript-decorators already has extra definition regarding property initializers https://github.com/wycats/javascript-decorators/blob/master/INITIALIZER_INTEROP.md .

The main concern there that property initializers is a usual descriptor, as well as class methods, so decorators can wrap them as well. I don't see reasons to worry, while spec in that section remains unchanged

— Reply to this email directly or view it on GitHub https://github.com/jashkenas/backbone/issues/3560#issuecomment-142015454 .

gautamborad commented 8 years ago

Just wanted to know the pros/cons on using https://github.com/typhonjs/backbone-es6 vs the method technique suggested by @benmccormick.

Btw, thanks @benmccormick for the excellent blog post!

dsheiko commented 8 years ago

In addition to the Proposal (#121) pull-request attaching here properties method in action https://github.com/dsheiko/backbone-abstract/tree/master/demo-es6/src/Js As @akre54 mentioned Justin has already proposed a similar solution (preInitialize method). While already using it on my branch, it really solves the problem to me. Appeared to be also useful in TypeScript despite they don't ban declarative class properties.

P.S. preInitialize sounds more general and therefore better in this context. Albeit, it more like preConstruct if we call the method prior all the constructor's jobs

tbranyen commented 8 years ago

Really wish we'd see a new class properties proposal that sets them on the prototype. It seems that many involved with the proposal are worried about the implications, but I find it incredibly inconsistent that class methods get directly attached to the prototype, while jeffmo's proposal puts them in the constructor.

Had they gone with attaching properties directly to the prototype you'd be able to migrate pretty much any React/Backbone code to ES2015 classes.

davis commented 8 years ago

awesome blog post @benmccormick !! going to use those decorators in my project

amiller-gh commented 8 years ago

@benmccormick, I whipped up with another way to declare classes with default properties, take a look: https://github.com/epicmiller/es2015-default-class-properties

It runs normally in any environment that natively supports classes, transpiles well, and looks far nicer than defining them in the constructor or after the declaration. With proposals for decorators and class properties coming down the pipeline for ES2016/ES2017 this may be more of an academic exercise than the long term solution for Backbone, but something like this is definitely a viable option if 2-3 years is too long of a wait.

tofagerl commented 8 years ago

Well, the thing is that Class Properties is still at stage 1 in the Ecmascript proposal stage system. I have no idea why, since it seems like a gimme in terms of "what the user gets". Of course, I have no idea what sort of things it might break under the hood both syntactically and in terms of reference implementations.

https://github.com/tc39/ecma262 https://github.com/jeffmo/es-class-fields-and-static-properties