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

Pluralizing the word 'gas' #91

Open
corrspt opened this issue Oct 22, 2015 · 17 comments
Open

Pluralizing the word 'gas' #91

corrspt opened this issue Oct 22, 2015 · 17 comments
Labels

Comments

@corrspt
Copy link

corrspt commented Oct 22, 2015

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
Copy link
Member

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
Copy link
Author

corrspt commented Oct 22, 2015

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
Copy link
Member

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
Copy link
Author

corrspt commented Oct 22, 2015

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
Copy link
Author

corrspt commented Oct 22, 2015

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
Copy link
Contributor

@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
Copy link
Author

corrspt commented Oct 22, 2015

@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
Copy link
Contributor

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

@corrspt
Copy link
Author

corrspt commented Oct 22, 2015

Thanks for helping!

@olivia
Copy link
Contributor

olivia commented Oct 22, 2015

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
Copy link
Author

corrspt commented Oct 22, 2015

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
Copy link
Contributor

olivia commented Oct 22, 2015

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
Copy link
Author

corrspt commented Oct 22, 2015

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
Copy link
Contributor

@corrspt can you post the server payload?

@corrspt
Copy link
Author

corrspt commented Oct 23, 2015

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
Copy link
Contributor

@corrspt can you give me access to the repo?

@fsmanuel
Copy link
Contributor

@corrspt lets move the conversation to slack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants