Make Model#_clone return instance of current type

[greim]

If I subclass Model like so:

class MyModel extends Model { ... }
var model = new MyModel().batch();

batch() nevertheless returns an instance of Model. This PR makes it so batch() and other cloning methods would return instances of MyModel.

[jhusain]

Just curious: why would you subclass model? Presumably there are many methods which return models which would also have to return the subclassed type. I would prefer to think of this class as sealed until we can think through all the implications. It would really help me if you could help me understand your use case.

[trxcllnt]

@jhusain I've subclassed model to add an async invalidate, an inspect method, and overrides that all convert their path syntax arguments to arrays and return RxJS-5 Observables.

[trxcllnt]

I also overrode _clone to return my subclass, so at least it's possible.

[greim]

My use case was to have a model with a getValueSync() method which returns a value from the cache so that I can use the Falcor model directly as a store, ALA this thread: #572

If you're mulling the best approach please don't feel pressured to merge this PR. I'm more keenly interested in the outcome of the above issue thread.

[steveorsomethin]

Sounds like there's use cases for this, and I don't see any negatives. Gonna merge tomorrow. Any objections, @jhusain?

[jhusain]

Primary concern is that I don't want to encourage subclassing to add a method to the Model. I'm reluctant do this because developers should treat Models like values, not objects. Subclassing the Model doesn't play well with the Model's compositional API. For example, what happens when you subclass a Promise? Does then() return MySubclassedPromise? Relying on overriding _clone to get this right depends on a private API. The compositional nature of the Model API suggests developers just pass Models to functions rather than add instance methods and try and override any method that returns a Model to preserve the subclass prototype. That's why I'm opposed to merging this PR.

…

Sent from my iPhone

On Aug 29, 2017, at 9:10 AM, Steve McGuire ***@***.***> wrote: Sounds like there's use cases for this, and I don't see any negatives. Gonna merge tomorrow. Any objections, @jhusain? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[trxcllnt]

@jhusain subclassing to override the core methods to return Rx Observables/automatically parse path syntax etc. has been useful. Supporting this let me remove that stuff from the core, but still get the features in the apps where we can afford the extra cost. Just some feedback from using falcor in the wild, obviously do whatever in this project.

[asyncanup]

A relevant discussion point here might be that Router does explicitly encourage subclassing (for performance gains): https://netflix.github.io/falcor/documentation/router.html#creating-a-router-class

[jhusain]

I’m not sure that the Router API is relevant, because the Router and the Model are used very differently. I think the central question here is this: do we want to make the Model safe to subclass? This decision is not free. It would limit our ability to move away from a constructor to a factory-based API in the future, because such a move could impose large migration costs on developers which took advantage of an API we provided.

We should also consider what message this API sends to our users. API design is one of the most effective ways we have of communicating with our users and pushing them into the “pit of success”. As long as Models are sealed, we can ensure they are cheap to create, which in turn promotes the correct use of Models. Enabling subclassing allows developers to undermine this guarantee, which could cause them to be much more precious about their Models than we intend. There are also other questions to think about and answer. Do subclasses have access to Model internals (ie are any members protected)? If not, why not?

It’s important to note that the real ask here does not appear to be able to subclass a model in the traditional sense (i.e. add additional state, write polymorphic implementations of methods). Instead it’s my perception that subclassing is simply a means by which to invoke stateless helper methods as though they were instance methods on the Model (please correct me if I’m wrong @greim). This is very reasonable given that this...

value.do().this()

...is a lot more readable than this:

this(do(value));

While I sympathize with this ask, it is a problem which all JavaScript frameworks suffer from equally. The root problem is a language problem: JavaScript privileges instance methods with a more ergonomic syntax. It’s my belief that this problem should be solved at the language level rather than the framework level (pipeline operator proposal https://github.com/tc39/proposal-pipeline-operator). Arguably this can already be accomplished relatively easily using ES2015 proxies.

Allowing Models to be subclassed in order to solve a JavaScript ergonomics problem opens a very wide door. How do we explain that we’re opening up subclassing, but don’t expect it to be used for any purpose but calling helper methods? There are a lot of questions to be answered here, and ultimately I think our efforts are best spent elsewhere.

[jhusain]

@greim Wanted to add that if your main use case is to your invoke helper methods as if they're members of Model, this has been enabled with ES6 Proxies. You can create a Proxy for the Model, intercept incoming calls to your helper methods, and dynamically implement them. While not as ergonomic as the pipeline operator, this should allow you to call helper methods you own as if they were instance methods on the Model. Remember to intercept calls to deref and batch to make sure the Model returned from those methods is also wrapped in your ES6 Proxy.

We got together and discussed this in more detail. The team collectively decided that we wanted to encourage developers to think of Models as cheap proxies, and consequently we want to discourage inheriting from Model. Under the circumstances we're moving away from constructors to factory functions in forthcoming versions.

[asyncanup]

@greim
After further discussion, we have decided to re-open this and merge it in.
The idea is not to encourage subclassing, but to still fix this pitfall that you fell into, and others may too.
Hope this helps!

[coveralls]

Coverage increased (+1.1%) to 93.19% when pulling dbac3e4 on greim:extend-clone into 313a859 on Netflix:master.

[coveralls]

Coverage increased (+1.1%) to 93.19% when pulling dbac3e4 on greim:extend-clone into 313a859 on Netflix:master.