unregistering then re-registering a model doesn't clear the factoryCache

[Leooo]

copy of emberjs/ember.js#15268

failing test: #4999

[rwjblue]

Ember Data is doing its own caching around factoryFor and therefore has another layer that needs to be busted (which cannot be done by owner.unregister). See store._modelFactoryFor implementation here.

There are a few different possibilities here that I can think of:

1. Expose a store.unregisterModelFactory method or something to bust the store's internal model factory cache.
2. Remove the secondary caching layer and rely on owner.factoryFor's internal caching.
3. Do not support changing a registered model after any instances have been created.

I personally am in the camp of both 2 and 3 😸, but there are likely good reasons for the extra layer of caching (e.g. to support .modelName system).

[rwjblue]

@runspired may have more insight round item 2 above (e.g. if we do need the secondary caching level)...

[runspired]

I don't believe we should allow cache-busting in ember-data for modelFor. I left a few comments here: #5443

TL;DR In the case described in the original Ember issue above, the following refactor would be better.

before

import MyMixin from '<app-name>/mixins/my-mixin';

moduleForModel('appointment', {
  integration: true,
  beforeEach() {
    let store = this.store();
    let MyModel = store.modelFor('my-model');
    let MyModel2 = MyModel.extend(MyMixin);
    getOwner(store).unregister('model:my-model');
    getOwner(store).register('model:my-model', MyModel2);
    let record = store.createRecord('my-model'); // BUG not extended
    record.get('aMixinProp')); // 'aMixinProp' is a property of the mixin, but is not available.
  }
});

after

import MyMixin from '<app-name>/mixins/my-mixin';

moduleForModel('appointment', {
  integration: true,
  beforeEach() {
    let store = this.store();
    let owner = getOwner(store);
   
    // we lookup the factory via the owner, not via modelFor
    let MyModelFactory = owner.factoryFor('model:my-model');
    let MyModel2 = MyModelFactory.klass.extend(MyMixin);
    
    owner.unregister('model:my-model');
    owner.register('model:my-model', MyModel2);
 
    // test is out :)
    let record = store.createRecord('my-model'); // will be extended
    record.get('aMixinProp')); // 'aMixinProp' is a property of the mixin, and will be available.
  }
});

[runspired]

Basically the reason I don't think we should cache-bust modelFor is that once that has been called it is extremely likely that we've

• cached attribute info
• cached relationship info
• created instances

The caching of attributes and relationships means that even using reopen would likely fail to work as expected. It would be a mega-troll all around, in addition to just being an odd pattern. Developers should work to ensure that their classes are correct prior to application instantiation, or generate completely new classes at runtime. Patching classes once they are already in use is perilous.

[runspired]

cc @rwjblue, unless you disagree I think we can close this since unregistering and then re-registering is an upstream concern (and has been cleaned up there).

[rwjblue]

I tend to agree.

[runspired]

Closing as wont-fix. Many thanks to @Leooo for reporting and for taking the time to write a test for us to discuss this with.

[Leooo]

Thanks for showing the proper fix @runspired. That helps

[Leooo]

Note to anyone it may help: runspired code above works, with one typo:

let MyModel2 = MyModelFactory.klass.extend(MyMixin);

=>

let MyModel2 = MyModelFactory.class.extend(MyMixin);

(Ember 2.15)

[Leooo]

To keep track (even if hopefully this is only in an old ember-data version and solved in most recent ones), I found I had this issue in unit tests for models (in maybe some edge cases where I do several unit tests on models extended with or without some model mixins), even when not using modelFor explicitly in my beforeEach hooks it was still apparently being called in tests, and some factory.superclass.modelName values seems to have been persisting across unit tests.

Working fix is to add needs: ['store.service'] to the tests, then override the store service in a tests/dummy/app/services/store.js files (I work inside an addon), with the below:

import DS from 'ember-data';

function _preventSuperClassModelNamesDuplicates(factory) {
  if (factory.superclass) {
    factory.superclass.modelName = undefined;
    _preventSuperClassModelNamesDuplicates(factory.superclass);
  }
}

export default DS.Store.extend({
  modelFor() {
    const factory = this._super(...arguments);
    _preventSuperClassModelNamesDuplicates(factory);
    return factory;
  }
});