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

dasherize ALL the things: use dasherized model names everywhere #3033

Merged
merged 1 commit into from
May 14, 2015

Conversation

fivetanley
Copy link
Member

use dasherized model names everywhere / change typeKey -> modelName

Previously, we allowed container keys to be loosy-goosy and normalized
them in several places. This commit consolidates those efforts into
one place normalizeTypeKey.

typeKey is now referred to as modelName on snapshot instances and Model
classes. typeKey remains on both of these but is deprecated.

modelName is now a dasherized string, where previously it was
camelCased.

@fivetanley fivetanley force-pushed the use-dasherized-everywhere branch 2 times, most recently from e0238ad to 8c97324 Compare May 3, 2015 22:46
@igorT
Copy link
Member

igorT commented May 4, 2015

Tests fail?

@fivetanley
Copy link
Member Author

Tests fail?

Yeah, I rebased without re running, fixed the tests that broke. They were expecting a camelCased type when extracting, but it should be dasherized.

var key = type+":"+name;
retrieveManagedInstance: function(typeKey, name) {
var normalizedTypeKey = this._normalizeTypeKey(typeKey);
var key = normalizedTypeKey +":"+name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nitpick: spaces around +

@igorT
Copy link
Member

igorT commented May 5, 2015

Can we add tests that assert this is backwards compatible?

@igorT
Copy link
Member

igorT commented May 5, 2015

And by we, I meant the awesome @fivetanley :D

var camelize = Ember.String.camelize;

/**
All typeKeys are camelCase internally. Changing this function may
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelCase => dasherized?

@fivetanley fivetanley force-pushed the use-dasherized-everywhere branch 3 times, most recently from c77c090 to c6de197 Compare May 10, 2015 20:14
@return {String} if the adapter can generate one, an ID
*/
export default function normalizeTypeKey(typeKey) {
return singularize(dasherize(camelize(typeKey)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to singularize here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have to, but it's nice

@igorT
Copy link
Member

igorT commented May 11, 2015

I am pretty scared of dasherizing typeKey, gonna break people's stuff a bunch. Maybe we should go with the modelName strategy?

@fivetanley
Copy link
Member Author

I am pretty scared of dasherizing typeKey, gonna break people's stuff a bunch.

It'd be great to know what people are using typeKey for. Most of the changes in your app code would be comparing against dasherized strings. If we're going to remove this, I'd rather remove it this week than right before 1.0.

@fivetanley
Copy link
Member Author

Maybe we should go with the modelName strategy?

Yeah, just takes more tedious work, but I can switch this over to that.

@typeoneerror
Copy link

It'd be great to know what people are using typeKey for.

I'm using typeKey in a handful of cases, specifically for some very hacky polymorhphic relationship handling. One example is the "attachable" polymorphic relationship. I have an action that updates the "attached" model when an image is removed from it. I use typeKey here (I'm sure there's a much better way to do this, but I just did a search in my project and that's where I'm using it):

imageRemoved: function(response, model) {
  let type = model.constructor.typeKey;
  if (response) {
    if (response.hasOwnProperty(type) && !Ember.isEmpty(response[type])) {
      let data = this.store.normalize(type, response[type]);
      this.store.push(type, data);
    }
  }
}

return match.toLowerCase();
}).replace('::', '/');
return this._super(normalized);
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _super is not RESTAdapter the return would not be present. It is, seems ok, but is not safe if the parent class is not as expected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@fivetanley fivetanley force-pushed the use-dasherized-everywhere branch 2 times, most recently from 25eca06 to 7de8109 Compare May 14, 2015 03:15
@fivetanley fivetanley force-pushed the use-dasherized-everywhere branch 3 times, most recently from d9609b0 to 153c4fd Compare May 14, 2015 03:30
@@ -25,7 +25,7 @@ function Snapshot(record) {
this.id = get(record, 'id');
this.record = record;
this.type = record.constructor;
this.typeKey = record.constructor.typeKey;
this.modelName = record.constructor.modelName;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs deprecation

Previously, we allowed container keys to be loosy-goosy and normalized
them in several places. This commit consolidates those efforts into
one place `normalizeTypeKey`.

typeKey is now referred to as modelName on snapshot instances and Model
classes. typeKey remains on both of these but is deprecated.

modelName is now a dasherized string, where previously it was
camelCased.
fivetanley added a commit that referenced this pull request May 14, 2015
dasherize ALL the things: use dasherized model names everywhere
@fivetanley fivetanley merged commit e97d2db into master May 14, 2015
@raytiley
Copy link

So this breaks everyone's apps that where model-name results in /modelname for the url.

@igorT
Copy link
Member

igorT commented May 14, 2015

@fivetanley I thought we kept the existing behavior for RestAdapter? None of the tests were changed

@raytiley
Copy link

@igorT looks like my app is customizing pathForType fixing it on my end.

@igorT
Copy link
Member

igorT commented May 16, 2015

@raytiley kinda surprised it broke, would you mind sharing what the problem/fix was?

@raytiley
Copy link

@igorT

I had this code which broke whith type='myModel' would now be my-models instead of mymodels.

RESTAdapter.reopen({
  pathForType: function(type) {
    return Ember.String.pluralize(type.toLowerCase());
  }
});

I changed it to

RESTAdapter.reopen({
  pathForType: function(type) {
    var path = this._super(type);
    return path.toLowerCase();
  }
});

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

7 participants