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

can not extend / override methods (for example _find) #3901

Closed
Leooo opened this issue Nov 1, 2015 · 7 comments
Closed

can not extend / override methods (for example _find) #3901

Leooo opened this issue Nov 1, 2015 · 7 comments

Comments

@Leooo
Copy link

Leooo commented Nov 1, 2015

Hello,

I have the following use case: using two stores, one offline with LocalStorageAdapter, the other one online with ActiveModelAdapter, and something close to EmberSync to link both worlds.

When fetching records from the server, I don't always embed their relationships to improve the performance, so I'm storing offline records with relationship ids which are not fetched yet.

When it's time to load the relationships, offline first, Ember can not find them offline and unloads the empty records following https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/store/finders.js, _find method l.36/37

I would like in some cases to not unload the empty records so that I'm able to access their ids and fetch them on the server, but here is the problem: impossible to override the _find method in https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/store/finders.js

I understand that in most cases, private methods are to be private and not giving access ensures they stay so, still for an open source framework, it is surprising to not be able to extend / override important classes / methods.

In my case for now, I hacked a whole bunch of other methods (store._findByInternalModel with different cases, depending on if we are using a store.findRecord or a model.get(relationship) etc.) to manage to satisfy my use case, but this is far dirtier than just overriding _find: As of now I'm creating loading records with corresponding deferred promises when getting relationships which are not yet fetched, which will be resolved once the server sends the results (not in the same loop, so I had to override flushPendingFetchFortype too..). But that means I have to take care when loading my relationships to possibly stuck pending promises, and make sure the corresponding records are fetched from the server - it would be better to send resolved / rejected promises when the record doesn't exist offline, as long as I'm still able to take note of the id to be fetched (not possible now with _find's unloadRecord).

Not sure my exact problem is clear, so to summarize: why can't finders store methods and others (I'm thinking about relationship helpers too) be made accessible / extendable by an Ember App.

Said another way: I have no clue how to extend ember$data$lib$system$store$finders$$_find, and I don't think this is possible, while I know how to do import X from ember-data/lib/system/store/finders for example. Is there a way to build ember-data in a way which would be more friendly for those who need to extend it?

Thanks

@bmac
Copy link
Member

bmac commented Nov 2, 2015

Hello @Leooo,

I think this is an interesting use case but I'm not convinced that exposing Ember Data's internal finder methods is the best way to solve this problem. Ember Data's internals tend to change over time as new features are added and performance improvements are implemented. This causes code that is coupled to these internals to be brittle and break when new version of Ember Data get released. An alternate solution may be to identify what sort of public API(s) Ember Data could to expose to help you solve your problem without needing to access Ember Data's private methods.

I'd like to know some more about your use case using multiple stores. Do you mind sharing a high level overview of how you are fetching records offline and when you want Ember Data to attempt to access the records via the network?

@Leooo
Copy link
Author

Leooo commented Nov 2, 2015

It is very dirty as I have to deliver on my website and I can't spend the time needed to find the right solution (especially without being able to test other solutions based on modifying ED internals). I send different adapterOptions from the offlineStore methods on find-like methods, so that I build loading records instead of rejecting promises when getting relationships only (not for normal findRecord methods) :

export default DS.Store.extend({
  //..
  findMany: function (internalModels) {
    var store = this;
    return Ember.RSVP.Promise.all(internalModels.map(function (internalModel) {
      return store._findByInternalModel(internalModel,{adapterOptions: {dontBuildRecord: true}});//dontBuildRecord is to differentiate from the _findByInternalModel of ember$data$lib$system$relationships$state$belongs$to$$BelongsToRelationship.prototype.findRecord / and equivalent hasMany
    }));
  },
  findRecord: function(modelName, id, options) {
    options = options || {};
    options.adapterOptions = options.adapterOptions || {};
    options.adapterOptions.dontBuildRecord=true;//dontBuildRecord is to differentiate from the 
    //_findByInternalModel of ember$data$lib$system$relationships$state$belongs$to$$BelongsToRelationship.prototype.findRecord / and equivalent hasMany
    return this._super(modelName, id, options);
  },
  fetchRecord: function(internalModel, options) {
    var prom=this._super(internalModel, options);
    var prom2=new Promise(function(resolve,reject){
      prom.then(function(res){resolve(res)},function(err){
        if (!(options.adapterOptions && options.adapterOptions.deferLoading)) {
          reject(err);
        }//otherwise, we keep the promise deferred so that the record stays loading
      })
    });
    return prom2;
  },
  flushAllPendingFetches: function () {
    if (this.isDestroyed || this.isDestroying) {
      return;
    }
    this._pendingFetch.forEach(this._flushPendingFetchForType, this);
    //LV: we want to keep the _pendingFetch for empty records (will resolve when fetched from server)
    //this._pendingFetch = Ember.Map.create();
  },
  _flushPendingFetchForType: function (pendingFetchItems, typeClass) {
    var store = this;
    var adapter = store.adapterFor(typeClass.modelName);
    var shouldCoalesce = !!adapter.findMany && adapter.coalesceFindRequests;
    var records = Ember.A(pendingFetchItems).mapBy("record");

    function _fetchRecord(recordResolverPair) {
      var prom=store.fetchRecord(recordResolverPair.record, recordResolverPair.options);
      var resolvedProm=function(res){
         pendingFetchItems.removeObject(recordResolverPair);
        recordResolverPair.resolver.resolve(prom);
      }
      //LV: dont remove _pendingFetch for loading records (wait for fetch from server)
      prom.then(resolvedProm,resolvedProm);
      //recordResolverPair.resolver.resolve(store.fetchRecord(recordResolverPair.record, recordResolverPair.options)); // TODO adapter options
    }

    if (pendingFetchItems.length === 1) {
      _fetchRecord(pendingFetchItems[0]);
    } else if (shouldCoalesce && false) {//LV, removed cause it takes hours to copy the needed code 
    } else {
      pendingFetchItems.forEach(_fetchRecord);
    }
  },
  createLoadingRecord: function(modelName0,id){
    //when a relationship has ids without corresponding records, we are creating corresponding loading records
    //so that we can access the ids (for emberSync.getRelationship)
    var self=this;
    var modelName=modelName0.camelize();
    var internalModel=this._internalModelForId(modelName,id);
    if (internalModel.isEmpty()) {
      var modelClass=this.modelFor(modelName0);
      var pendingItems=this._pendingFetch.get(modelClass);
      var isAlreadyLoading=false;
      if (pendingItems) {
        //timer_begin("isAlreadyLoading");//performance of filter??
        var filter=pendingItems.filter(function(item){return item.record.id===id;});
        if (filter.length) {
          isAlreadyLoading=true;
          internalModel=filter.get("firstObject.record");
          //self.log("black",modelName,internalModel.id)("isAlreadyLoading",internalModel);
        }
        //timer_end("isAlreadyLoading");
      }
      /*if (isAlreadyLoading) {//no need: _findByInternalModel => _findEmptyInternalModel will return the _loadingPromise
        return internalModel._loadingPromise;
      }
      else {*/
        //promiseObject, will return a normal record when resolving
        return this._findByInternalModel(internalModel,{adapterOptions: {deferLoading: true,allowRecursive: false}});
      //}
    }
    else {
      if (internalModel._loadingPromise) {
        return internalModel._loadingPromise.then(function(rec){return rec.getRecord();});
      }
      return Ember.RSVP.resolve(internalModel.getRecord());
    }
  },
}

function deserializeRecordId(store, key, relationship, id) {
  if (Ember.isNone(id)) {
    return;
  }
  var type=id.type.modelName || id.type;
  var internalModel=store._internalModelForId(type, id.id);
  if (internalModel.isEmpty()) {//LV
    var prom=store.createLoadingRecord(id.type,id.id);
  }
  return internalModel;
}

Then in my localStorage adapter, I use the buildRecord options to sometimes build a loading record:

export default {
  name: 'override-local-storage-adapter',
  before: 'store',
  initialize: function() {
    DS.LSAdapter.reopen({
      //..
      findRecord: function(store, modelClass, id, opts) {
        var allowRecursive = true;
        var namespace = this._namespaceForType(modelClass);
        var record = Ember.copy(namespace.records[id],true);
        //this.log("black",modelClass.modelName,id)("findRecord "+modelClass.modelName+" "+id,record,opts);
        if (opts && typeof opts.allowRecursive !== 'undefined') {
          allowRecursive = opts.allowRecursive;
        }
        if (opts && opts.adapterOptions && typeof opts.adapterOptions.allowRecursive !== 'undefined') {
          allowRecursive = opts.adapterOptions.allowRecursive;
        }
        if (!record || !record.hasOwnProperty('id')) {
          //LV: build empty record when loading relationship
          var buildRecord=true;//for ember$data$lib$system$relationships$state$belongs$to$$BelongsToRelationship.prototype.findRecord for example
          if (opts && opts.adapterOptions && opts.adapterOptions.dontBuildRecord) {buildRecord=false;}//store's findRecord/findMany methods
          if (opts && opts.adapterOptions && opts.adapterOptions.buildRecord) {buildRecord=true;}//for loadRelationships
          if (buildRecord) {
            var prom=store.createLoadingRecord(modelClass.modelName,id);
            var serializer = store.serializerFor(modelClass.modelName);
            return prom.then(function(res){
              return res._createSnapshot();
            },function(err){
              //self.log("red",modelClass.modelName,id)("error in findRecord createLoadingRecord promise rejected",err,prom);
            });
          }
          else {
            return Ember.RSVP.reject();
          }
        }

        if (allowRecursive) {
          return this.loadRelationships(store,modelClass, record,opts);
        } else {
          return Ember.RSVP.resolve(record);
        }
      },
      //and equivalent for findMany
      loadRelationships: function(store,modelClass, record,opts) {
          //..
          //LV
          opts = opts || {};
          opts.allowRecursive= false;
          opts.adapterOptions = opts.adapterOptions || {};
          opts.adapterOptions.allowRecursive= false;
          if (!opts.adapterOptions || !opts.adapterOptions.dontBuildRecord) {
            opts.adapterOptions.buildRecord=true;
          }
          if (relationEmbeddedId && DS.LSAdapter.prototype.isPrototypeOf(adapter)) {
            recordPromise = recordPromise.then(function(recordPayload) {
              var promise;
              if (relationType === 'belongsTo' || relationType === 'hasOne') {
                //LV
                //promise = adapter.find(null, relationModel, relationEmbeddedId, opts);
                promise = adapter.findRecord(self.store, relationModel, relationEmbeddedId, opts);
              } else if (relationType == 'hasMany') {
                //promise = adapter.findMany(null, relationModel, relationEmbeddedId, opts);
                promise = adapter.findMany(self.store, relationModel, relationEmbeddedId, opts);
              }
              return promise.then(function(relationRecord) {
                return adapter.addEmbeddedPayload(recordPayload, relationName, relationRecord);
              },function(error){
                //self.log("black",relationName)("catch loadRelationships happens when findRecord opts.adapterOptions.dontBuildRecord",
                                              relationEmbeddedId, recordPayload,modelClass, record,opts);
                return recordPayload;
              });
            });
          }
        });
        return recordPromise;
      },
  }
}

In DS.Model.extend:

  hasLoadingRelationship: function(relationshipName) {//general method to know if a model.get(relationship) is loading or not
    var loadingRec=this.get(relationshipName+".content");
    if (!loadingRec) {return false;}
    var loadingRecs=Ember.isArray(loadingRec) ? loadingRec : [loadingRec];
    var isLoading=false;
    loadingRecs.forEach(function(rec){
      isLoading=isLoading || rec.get("isLoading");
    });
    return isLoading;
  },

Finally, in the emberSync mixin, I created a method modelInstance.emberSync.getRelationship(relationshipName) to be used instead of modelInstance.get(relationshipName), where if modelInstance.hasLoadingRelationship is true, then I fetch the relationships from the server (using the id(s) of the loading record(s)), and if not then I send normal modelInstance.get(relationshipName).

@Leooo
Copy link
Author

Leooo commented Nov 2, 2015

I understand your explanation on why private methods are private. Still, I don't know how users can understand what ember-data does without being able to extend it for testing / logging purposes.

@Leooo
Copy link
Author

Leooo commented Nov 2, 2015

I haven't been able to test this as I don't want to change ember-date in bower, but I think what could be changed is:

  • Ability to send an options.dontUnloadEmptyRecords in _find() so that empty records are not unloaded.
  • Use this option for get(relationshipName) only (not for normal findRecord)
  • Then in a get(relationshipName) routine, reject the promise if there are empty records, but leave them in content so that we are able to fetch them from the server.

?

@locks
Copy link
Contributor

locks commented Jan 6, 2017

Hi @Leooo, is this still relevant?

@Leooo
Copy link
Author

Leooo commented Jan 6, 2017

@locks probably, but i suppose noone has time for it so you can close.

L

@locks
Copy link
Contributor

locks commented Jan 6, 2017

@Leooo hopefully the newer APIs and refactors help with this issue. Thanks for reporting, and let us know if you bump against this again!

@locks locks closed this as completed Jan 6, 2017
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

No branches or pull requests

3 participants