Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't discard return value of default constructors. (with tests) #1551

Closed
wants to merge 1 commit into from
Closed

Conversation

valueof
Copy link

@valueof valueof commented Aug 14, 2012

If you try to transform a Backbone class into an implicit factory
(i.e. a class that acts as a factory even when used with new) you
must have a reference to an object created by the default constructor.

This patch makes Backbone to not to discard return value of default
constructors. In addition to the scenario described above it also
makes both code paths consistent with each other.

This is a version of GH-1435 and GH-1429 (but with tests; not sure why
all other PRs didn't have tests) and an inverse of GH-1382.

If you try to transform a Backbone class into an implicit factory
(i.e. a class that acts as a factory even when used with `new`) you
must have a reference to an object created by the default constructor.

This patch makes Backbone to not to discard return value of default
constructors. In addition to the scenario described above it also
makes both code paths consistent with each other.

This is a version of GH-1435 and GH-1429 (but with tests; not sure why
all other PRs didn't have tests) and an inverse of GH-1382.
@wookiehangover
Copy link
Collaborator

seems like a reasonable addition to me, especially since multiple issues have been opened up about it in the past few months...

@braddunbar @jashkenas curious to hear your take on getting this landed.

@jashkenas
Copy link
Owner

Not allowing a constructor to change the type of the returned object is more of a feature than a bug. Instead of "transforming a Backbone class into an implicit factory" ... just do this:

var factory = function(attrs) {
  return new MyModel(attrs);
};

@valueof
Copy link
Author

valueof commented Aug 15, 2012

@jashkenas I'd love to but then you can't use collections and such because they often automatically create model instances. In case of Disqus this results in multiple memory leaks that we're now trying to fix. It'd be awesome to have a simpler solution though—so if you know something from your experience I'd love to hear that.

@joesteele
Copy link

In a few cases, I just override the model attribute of a few of our collections to be a factory.

So in CoffeeScript...

class SomeCollection extends Backbone.Collection
  model: (attrs, options) ->
    if model = KnownModelTypes[attrs.type]
      return new model(attrs, options)
    else
      return new GenericModel(attrs, options)

Is something like that what you meant?

@jashkenas
Copy link
Owner

Yep, exactly.

Backbone.Collection.extend({
  model: modelFactoryFunction
});

@valueof
Copy link
Author

valueof commented Aug 15, 2012

That's way more elegant. Thanks @joesteele and @jashkenas!

Closing this PR. But the consistency question is still up. GH-1382 to the rescue?

@valueof valueof closed this Aug 15, 2012
@jashkenas
Copy link
Owner

I'm a little iffier on that one. It's one thing for our default implementation to (IMO) do the right thing re: constructors ... but it's another thing for us to wrap your chosen constructor function, just to prevent you from returning from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants