adopted-ember-addons / ember-data-factory-guy

Factories and helper functions for (unit, integration, acceptance) testing + development scenarios with Ember Data
MIT License
301 stars 136 forks source link

Compatibility with ember-data-model-fragments #182

Closed whatthewhat closed 8 years ago

whatthewhat commented 8 years ago

Hi, first of all great work with the library, really like the latest changes!

Currently there is an issue when trying to use latest factory-guy versions with ember-data-model-fragments. It has to do with looking up transforms here, ember-data-model-fragments monkey patches the transformFor on the serializer to do it's magic and so when factory-guy tries to find the transform with container.lookup if fails with an error.

Admittedly this is not exactly a factory-guy problem, but it would be nice if the 2 libraries could work together again and it does not seem like ember-data-model-fragments can stop depending on transformFor any time soon.

So the question is, would a PR with a change similar to this be accepted (even though it basically adds a dependency on a private serializer method)?

  getTransformValueFunction(type, modelName) {
    // ...
    let serializer = this.store.serializerFor(modelName);
    if ('function' == typeof serializer.transformFor) {
      return serializer.transformFor(type);
    } else {
      // use existing container lookup
    }
  }

Link to the discussion in the ember-data-model-fragments repo: https://github.com/lytics/ember-data-model-fragments/issues/185

danielspaniel commented 8 years ago

I think this would be an ok hackeroo because as long as it does not affect the usual factory guy hokus pokus ( the tests all still pass ) and it's labelled ( "note the hack here because of model fragment" ) then everyone knows what is going on and no one will know the difference

whatthewhat commented 8 years ago

Great! I'll submit a PR some time this week.

patocallaghan commented 8 years ago

@whatthewhat just curious if you have tried out this fix? I tried it out but it then seemed to fail further down the line with another error. I'll dig it out when I get the chance.

whatthewhat commented 8 years ago

@patocallaghan Did not have time to test this yet, unfortunately. Will definitely play with it on the weekend, let me know if you find anything!

mattmcginnis commented 8 years ago

I created an example repo If anyone wants to take a look.

danielspaniel commented 8 years ago

@patocallaghan , @whatthewhat , @mattmcginnis I just "fixed" model fragment factories to no longer output an id.

Not sure if you even care, but when using factories with fragments there was always an id put into the fixture. I noticed, but I did not care until one day I was doing something and I realized:
"By golly there is an id in my fragment data"
because one of my tests was not loving that very much, and I realized I should stop doing that. So in v2.7.0-beta.11 and onwards .. unless someone says otherwise:

The id is now only used for building ( see release notes ) but removed afterwards.