Improve default model handleing of serialize hook

[southpolesteve]

Previous discussion here: #2933
And here: http://discuss.emberjs.com/t/improving-the-route-serialize-behavior/1956

Finally got a chance to write up a proper PR with an additional test for the new behavior

[southpolesteve]

As mentioned by the @drogus in the discuss.ember thread, one thing to consider is calling camelize() on the params by default since camel case property names are considered best practices

[wagenet]

Looks like there is some tabbing inconsistency here.

[wagenet]

This is interesting, but I feel like we'd need this to correspond with the model method as well. We need to support a full round trip.

[drogus]

@wagenet you mean that you would like to use those params in order to fetch a model? I would not think of it as a blocker for this PR, it's tricky to fetch model by something else than id and I'm not sure we should do it.

[southpolesteve]

@wagenet fixed the tabbing issue. Thanks for catching!

I see what you are saying about a corresponding change to model, but I agree with @drogus that it doesn't seem like a blocker for this PR. I could open another PR with a potential model change. Do you imagine it would execute a Model.find(params) triggering a findQuery?

[machty]

I think we can merge this in; I feel like there's already some asymmetry between model and serialize given their inherent limitations of what they can surmise about the data they're provided, and this PR does a good job of supplying the more expected behavior.

The one thing that's strange is that a lot of people still think that routes with multiple dynamic segments have multiple models, but in practice this is pretty tricky since it's hard to construct links to these routes via the linkTo helper without having to go through the hassle of constructing the expected model parameter in a computed property. I think this concern is outside the scope of this PR though.

[wagenet]

This should check for getProperties as well. It's not a guarantee that the model is an Ember.Object.

[southpolesteve]

I think Ember.typeOf will only return instance for Ember.Object

https://github.com/southpolesteve/ember.js/blob/8c71bfa2478625958f0e3bc49a7ba6684ca9aae8/packages/ember-metal/lib/utils.js#L600

Would it be better to get rid of the Ember.typeOf(model) === "instance" check completely and only check that getProperties is defined?

In any case, do you think it is better to check if getProperties is a function?

Ember.typeOf(model.getProperties) === 'function'

Or just that it exists?

Ember.typeOf(model.getProperties) !== 'undefined'

[machty]

I would much prefer something like this:

// at top of file
var getProperties = Ember.Object.prototype.getProperties;

// replace `model.getProperties()` with:
getProperties.call(model, params);

I've verified that this works and is robust against weird/falsy values for model

[southpolesteve]

@machty cool. Just pushed that change.

[machty]

Hey, sorry, but turns out we're adding in Ember.getProperties in a few moments. I'll let oyu know when that's in, and then you can rebase to use that instead. Folk think it's weird to pull getProperties off of Object's prototype, and I agree

[machty]

@southpolesteve home stretch! 343166e just merged; replace the Ember.Object.prototype.getProperties with Ember.getProperties and i think we can merge. Thanks!

[machty]

So sorry to drag this on, but given that getProperties now works on POJOs, can't we just get rid of the Ember.typeOf(model) === "instance" check and just call getProperties(model, params) in the else case?

[wagenet]

@machty I think if you want multiple models, you should nest your routes, so I'm not too worried about that case. I think I'm ok to merge this once the one issue I pointed out is fixed.

[southpolesteve]

@machty updated to use Ember.getProperties

[southpolesteve]

@machty your right! No need for that check anymore. Much simpler now. Pushed and updated.