emberjs / ember-inflector

ember-inflector goal is to be rails compatible.
MIT License
106 stars 81 forks source link

Pluralizing the word 'gas' #91

Open corrspt opened 9 years ago

corrspt commented 9 years ago

Hi!

I've been working on an Ember Project (EmberCLI 1.13.8, Ember 2.0, Ember Data 2.0), and I've created a model 'gas'.

When using ember-data to query the store. this.store.query('gas') the url that's built is /api/gasinstead of what I expected, which would be /api/gases. I've created a custom adapter to deal with the situation, but I think this might be a problem with the ember-inflector.

I've created an Ember Twiddle to show the issue

Any thoughts? Thanks for the project!

stefanpenner commented 9 years ago

The default set of inflection aim for 1:1 with those from rails. If it handles gas differently we should adjust, otherwise the inflector has a public API for adding/remove the default inflection rules.

corrspt commented 9 years ago

Hey, thanks for the very quick reply.

I'm not a Rails guy, not sure how to check the Inflection for Rails to see where the problem is, sorry :(

Anyway, I've looked at the unit tests and it seems that you add "generic rules" for pluralization, and not for specific words.

test('ability to add additional pluralization rules', function(assert){
  assert.equal(inflector.pluralize('cow'), 'cow', 'no pluralization rule');

  inflector.plural(/$/, 's');

  assert.equal(inflector.pluralize('cow'), 'cows', 'pluralization rule was applied');
});

Would I have to do something like inflector.plural(/gas/,'es');?

Also, to make it available in the app, would I need an initializer? Something like this (copied from the ember guides)?

//
import Inflector from 'ember-inflector';

export function initialize(application) {
  Inflector.inflector.plural(/^gas$/,'gases');
  Inflector.inflector.singular(/^gas(es)$/,'$1');
};

export default {
  name: 'shopping-cart',
  initialize: initialize
};
stefanpenner commented 9 years ago

I'm about to get on 17h of flights. I will attempt to respond when I get settled back home.

Maybe @ofbriggs has some spare cycles.

It seems like we have some missing docs here...

corrspt commented 9 years ago

Oh, that's a really long flight! Hope you can get some rest!

Thanks for the very quick reply. I'll wait for feedback then.

If my guess from the tests is correct I can try to setup a PR with some documentation, but if anyone on the team has some guidelines for me, I'm here to help.

corrspt commented 9 years ago

I've tried to create the initializer like the following:

import Inflector from 'ember-inflector';

export function initialize(/* container, application */) {
  // Add a rule to the inflector to this the gas vs gases pluralization issue
  // https://github.com/stefanpenner/ember-inflector/issues/91
  Inflector.inflector.plural(/^gas$/,'gases');
  Inflector.inflector.singular(/^gas(es)$/,'$1');
}

export default {
  name: 'MyAPP',
  initialize: initialize
};

And it Kind of works, it's not making the request to /gases instead of /gas but when the data comes in, it read the type attribute as gases and it tries to singularize the name, it fails to gase. I've managed to see that the Inflector instance that my initializer is adding the plural/singular rule is not the same when the time comes to singularize this name. (Because the Inflector instance on the initializer has 22 and 28 rules for plural and singular, and when reading the JSON data it only has 21 and 27).

I've tried changing my backend model to 'gas' but now the singularize function is reading my model name as 'ga' (instead of 'gas'). Will continue to search for a solution in the mean time.

fsmanuel commented 9 years ago

@corrspt try Ember.Inflector.inflector instead of import Inflector from 'ember-inflector' that should be the instance ember data uses.

From the docs: http://guides.emberjs.com/v1.10.0/models/the-rest-adapter/#toc_pluralization-customization

var inflector = Ember.Inflector.inflector;

inflector.irregular('formula', 'formulae');
inflector.uncountable('advice');
corrspt commented 9 years ago

@fsmanuel Thanks for pitching in, I wasn't able to solve my problem with just that, but you led in a direction that seems to have solved the issue for now:

I've had to do the following:

import Ember from 'ember';

export function initialize(/* container, application */) {
  // Add a rule to the inflector to this the gas vs gases pluralization issue
  // https://github.com/stefanpenner/ember-inflector/issues/91
  // See the comments on the issue
  Ember.Inflector.inflector.plural(/^gas$/,'gases');
  Ember.Inflector.inflector.singular(/^gases$/,'gas');
}

export default {
  name: 'my-app',
  initialize: initialize
};

This makes my queries to the correct endpoints (/gases) but when ember-data reads the content it blows up like this:

TypeError: Cannot read property 'type' of null
    at ember$data$lib$system$store$$Service.extend._pushInternalModel (http://localhost:4200/assets/vendor.js:128937:29)
    at http://localhost:4200/assets/vendor.js:128918:27
    at Array.map (native)
    at ember$data$lib$system$store$$Service.extend.push (http://localhost:4200/assets/vendor.js:128917:42)
    at http://localhost:4200/assets/vendor.js:123833:27
    at Object.Backburner.run (http://localhost:4200/assets/vendor.js:10838:25)
    at ember$data$lib$system$store$$Service.extend._adapterRun (http://localhost:4200/assets/vendor.js:129152:33)
    at http://localhost:4200/assets/vendor.js:123830:15
    at tryCatch (http://localhost:4200/assets/vendor.js:60671:14)
    at invokeCallback (http://localhost:4200/assets/vendor.js:60686:15)
_pushInternalModel: function (data) {
    var _this4 = this;
    var modelName = data.type; // <--- Cannot read property 'type' of null is here

But if I add a custom serializer (to override the default which uses ember-inflector):

// pods/gas/serializer.js
import DS from 'ember-data';

export default DS.JSONAPISerializer.extend({
  modelNameFromPayloadKey(key) {
    if (key === 'gases'){
      return 'gas';
    }
    return this._super(...arguments);
  },

  serialize: function(snapshot, options) {
    var json = this._super.apply(this, arguments);
    json.data.type = 'gases';
    return json;
  }
});

It now appears to be working.

fsmanuel commented 9 years ago

Seems like a hack or bug to me. Will have a look at it later.

corrspt commented 9 years ago

Thanks for helping!

olivia commented 9 years ago

I've forked @corrspt's fiddle to use Ember.Inflector.inflector.irregular(...) as @fsmanuel suggested and it looks like it is working as expected. If you open the console you see that there is a call to /gases.

http://ember-twiddle.com/f33d67be2d6317a7695a

corrspt commented 9 years ago

Hi @ofbriggs thanks for pitching in!

Doing what @fsmanuel suggested did indeed got me past the the wrong endpoint query. But when the backend returned a JSONAPI response with type: 'gases' then ember-data would blow up at the _pushInternalModel method.

Because of that I had to override the serializer to not use the default inflector for 'gas' and use my exception. By doing this, I got things working as I expected. But still, the inflector instance that is being used by the default serializer is not affected by my initializer. I've debugged inside ember-data and saw that my new rules were not present in the inflector (when processing the json data from the backend that is).

olivia commented 9 years ago

Thanks for the extra information @corrspt :) That's quite odd that ED sometimes ignores the added irregular rules, I'll look into it why this is...

corrspt commented 9 years ago

I might have made a mistake somewhere else which leads to this situation. But I'm not sure where to find. Thanks for helping out, if you need anything else, do ask!

fsmanuel commented 9 years ago

@corrspt can you post the server payload?

corrspt commented 9 years ago

Sure, this is the result of asking /get/gases

{
  "data": [
    {
      "type": "gases",
      "id": "2",
      "attributes": {
        "symbol": "Comboda",
        "composition": "Compo"
      },
      "relationships": {
        "code": {
          "links": {
            "self": "http://localhost:9000/api/gases/2/relationships/code",
            "related": "http://localhost:9000/api/gases/2/code"
          },
          "data": {
            "type": "industry-codes",
            "id": "17"
          }
        },
        "specification": {
          "links": {
            "self": "http://localhost:9000/api/gases/2/relationships/specification",
            "related": "http://localhost:9000/api/gases/2/specification"
          },
          "data": {
            "type": "gas-specifications",
            "id": "3"
          }
        }
      },
      "links": {
        "self": "http://localhost:9000/api/gases/2"
      }
    },
    {
      "type": "gases",
      "id": "1",
      "attributes": {
        "symbol": "Symbol",
        "composition": "Teste"
      },
      "relationships": {
        "code": {
          "links": {
            "self": "http://localhost:9000/api/gases/1/relationships/code",
            "related": "http://localhost:9000/api/gases/1/code"
          },
          "data": {
            "type": "industry-codes",
            "id": "8"
          }
        },
        "specification": {
          "links": {
            "self": "http://localhost:9000/api/gases/1/relationships/specification",
            "related": "http://localhost:9000/api/gases/1/specification"
          },
          "data": {
            "type": "gas-specifications",
            "id": "1"
          }
        }
      },
      "links": {
        "self": "http://localhost:9000/api/gases/1"
      }
    }
  ],
  "included": [
    {
      "type": "gas-specifications",
      "id": "1",
      "attributes": {
        "specification": "MATATA"
      },
      "relationships": {},
      "links": {
        "self": "http://localhost:9000/api/gas-specifications/1"
      }
    },
    {
      "type": "gas-specifications",
      "id": "3",
      "attributes": {
        "specification": "BODING"
      },
      "relationships": {},
      "links": {
        "self": "http://localhost:9000/api/gas-specifications/3"
      }
    },
    {
      "type": "industry-codes",
      "id": "17",
      "attributes": {
        "code": "A",
        "region": "A"
      },
      "relationships": {},
      "links": {
        "self": "http://localhost:9000/api/industry-codes/17"
      }
    },
    {
      "type": "industry-codes",
      "id": "8",
      "attributes": {
        "code": "ISO",
        "region": "ISO"
      },
      "relationships": {},
      "links": {
        "self": "http://localhost:9000/api/industry-codes/8"
      }
    }
  ],
  "meta": {
    "records": 2
  }
}
fsmanuel commented 9 years ago

@corrspt can you give me access to the repo?

fsmanuel commented 9 years ago

@corrspt lets move the conversation to slack