amiel / ember-data-url-templates

an ember-addon to allow building urls with url templates instead of defining buildURL
https://github.com/amiel/ember-data-url-templates/wiki
MIT License
129 stars 22 forks source link

Remove re-implementation of pathForType #10

Closed courajs closed 8 years ago

courajs commented 8 years ago

The default ember adapter pluralizes the model type: https://github.com/emberjs/data/blob/v2.2.1/packages/ember-data/lib/adapters/build-url-mixin.js#L280 Re-implenting it in the mixin will clobber a user-specified path function from a super class. For example, this setup will erroneously pluralize the type:

// application/adapter.js
export default DS.RESTAdapter.extend({
  pathForType(type) {
    return Ember.String.camelize(type);
  }
});

// thing/adapter.js
import ApplicationAdapter from '../application/adapter.js';
import UrlTemplates from "ember-data-url-templates";
export default ApplicationAdapter.extend(UrlTemplates, {
  urlTemplate: "{+host}{/namespace}/{pathForType}{/id}"
});
courajs commented 8 years ago

Note: I built this PR on top of #9

amiel commented 8 years ago

This makes plenty of sense.

Note that this will break for people directly using the base system adapter nor the RESTAdapter, as I don't think they implement pathForType.

Would you:

  1. Rebase on master now that #9 is merged.
  2. At least look in to implementing pathForType with super and a deprecation warning. I'm thinking something like:

    // I'm pretty sure a missing super will just return undefined as opposed to error
    let pathForType = this._super(...); 
    if (pathForType) { return pathForType; }
    // A deprecation warning and the original implementation.
    // We will remove all of this in the 1.13 version.
courajs commented 8 years ago

Sure, I'll look into it

amiel commented 8 years ago

Thanks!

courajs commented 8 years ago

So, I noticed pathForType is implemented in the buildURLMixin: https://github.com/emberjs/data/blob/v2.2.1/packages/ember-data/lib/adapters/build-url-mixin.js#L255 Since what this addon does is override buildURL, it doesn't make sense for it to be used in a base adapter where the buildURLMixin isn't used. I think we can skip the deprecation. What do you think?

courajs commented 8 years ago

(I've rebased the branch)

amiel commented 8 years ago

@drspaniel good point. This looks good.