chore(store): cleans up and optimizes code paths around fetching rela… #4617

runspired · 2016-10-22T01:05:03Z

…tionships and single records

Also underscores some store methods that are private that should have been before.

pangratz · 2016-10-22T01:10:30Z

addon/-private/system/relationships/state/has-many.js

-    this._loadingPromise = promiseManyArray(this.store.scheduleFetchMany(manyArray.toArray()).then(() => {
-      return manyArray;
-    }), 'Reload with ids');
+    this._loadingPromise = promiseManyArray(this.store._scheduleFetchMany(manyArray.currentState).then(() => { return manyArray; }), 'Reload with ids');


() => manyArray

igorT · 2016-10-22T01:23:38Z

Seems good, I'll give it a once over once less tired

pangratz · 2016-10-22T01:26:23Z

addon/-private/system/store.js

    } else {
-      this._pendingFetch.get(typeClass).push(pendingFetchItem);
+      this._pendingFetch.get(modelClass).push(pendingFetchItem);


This if/else could be handled more elegantly using MapWithDefault...

It may also be better to just create a hash via modelName, I don't think we need to go all the way to a map here.

Now that we're using MapWithDefault, this can be shortened to this._pendingFetch.get(modelClass).push(pendingFetchItem).

stefanpenner · 2016-10-22T13:13:17Z

addon/-private/system/model/internal-model.js

    @private
    @param {Object} preload
  */
-  _preloadData(preload) {


lets keep the_

@stefanpenner I removed this one because InternalModel is "entirely private api" and this is used by other private modules, I believe that was our stated api naming goal?

runspired · 2016-10-22T14:14:02Z

@stefanpenner @pangratz cleaned up

pangratz · 2016-10-22T14:24:38Z

addon/-private/system/store.js


    internalModel.loadingData(promise);
+    let cache = this._pendingFetch.getWithDefault();


What does this?

#facepalm fixed @pangratz

pangratz · 2016-10-22T15:01:44Z

addon/-private/system/store.js

+
+    for (let i = 0; i < totalItems; i++) {
+      internalModels[i] = pendingFetchItems[i].internalModel;
+      seeking[internalModels[i].id] = pendingFetchItems[i];


Do we want to cache pendigFetchItems[i] so the lookup only happens once? This would also allow to get the internalModel's id from the cached result without looking it up again.

I debated on this, the alternative is:

let pendingItem = pendingFetchItems[i]; let internalModel = pendingItem.internalModel; internalModels[i] = internalModel; seeking[internalModel.id] = pendingItem;

This may be more clear but I'm not sure the "cache" actually is any simpler.

I would be in favor of better readability here

pangratz · 2016-10-22T15:02:30Z

addon/-private/system/store.js


    function _fetchRecord(recordResolverPair) {
-      recordResolverPair.resolver.resolve(store.fetchRecord(recordResolverPair.record, recordResolverPair.options)); // TODO adapter options
+      recordResolverPair.resolver.resolve(store._fetchRecord(recordResolverPair.internalModel, recordResolverPair.options)); // TODO adapter options


Let's split this into separate statements for better readability.

like so?

recordResolverPair.resolver.resolve( store._fetchRecord(recordResolverPair.internalModel, recordResolverPair.options) ); // TODO adapter options

I meant something like:

let fetchRecord = store._fetchRecord(…); recordResolverPair.resolver.resolve(fetchRecord);

pangratz · 2016-10-22T15:04:29Z

addon/-private/system/store.js

-            id: 'ds.store.missing-records-from-adapter'
-          });
+      // reject missing records
+      let missingRecords = [];


Rename to missingInternalModels to better reflect what they are containig

pangratz · 2016-10-22T15:05:12Z

addon/-private/system/store.js

+      let missingRecords = [];
+      for (let i = 0, l = expectedInternalModels.length; i < l; i++) {
+        if (!found[expectedInternalModels[i].id]) {
+          missingRecords.push(expectedInternalModels[i]);


Cache expectedInternalModels[i]?

same argument as above, I always go back and forth on this. Saving one index access vs one assignment. Perf wise it's likely ignorable, but readability is a bit better.

pangratz · 2016-10-22T15:05:56Z

addon/-private/system/store.js

-        rejectRecords(records, error);
-      };
-    }
+    function rejectRecords(internalModels, error) {


rejectInternalModels

pangratz · 2016-10-22T15:07:23Z

addon/-private/system/store.js

-            then(null, makeRecordsRejector(requestedRecords));
+        for (let j = 0; j < totalInGroup; j++) {
+          groupedInternalModels[j] = group[j]._internalModel;
+          ids[j] = groupedInternalModels[j].id;


Cache group[j]._internalModel

pangratz · 2016-10-23T10:11:47Z

addon/-private/system/store.js

-      resolver: resolver,
-      options: options
+    let modelClass = internalModel.type;
+    let resolver = Ember.RSVP.defer('Fetching ' + modelClass.modelName + 'with id: ' + internalModel.id);


Missing space after modelClass.modelName + 'with …'

runspired · 2016-10-27T15:14:55Z

@pangratz updated

…tionships and single records

runspired · 2016-10-28T19:11:54Z

@bmac rebased

pangratz reviewed Oct 22, 2016

View reviewed changes

stefanpenner reviewed Oct 22, 2016

View reviewed changes

runspired force-pushed the cleanup-many-array-reload branch from 63df671 to 106f965 Compare October 22, 2016 14:13

pangratz reviewed Oct 22, 2016

View reviewed changes

runspired force-pushed the cleanup-many-array-reload branch from 106f965 to 1ed3cc4 Compare October 22, 2016 14:30

pangratz reviewed Oct 22, 2016

View reviewed changes

runspired force-pushed the cleanup-many-array-reload branch from 1ed3cc4 to 7bd74df Compare October 22, 2016 15:48

pangratz reviewed Oct 23, 2016

View reviewed changes

runspired force-pushed the cleanup-many-array-reload branch from 7bd74df to 85934c6 Compare October 27, 2016 15:14

chore(store): cleans up and optimizes code paths around fetching rela…

37c33e2

…tionships and single records

runspired force-pushed the cleanup-many-array-reload branch from 85934c6 to 37c33e2 Compare October 28, 2016 19:11

bmac merged commit d0a3bbb into emberjs:master Oct 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(store): cleans up and optimizes code paths around fetching rela… #4617

chore(store): cleans up and optimizes code paths around fetching rela… #4617

runspired commented Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016

igorT commented Oct 22, 2016

pangratz Oct 22, 2016 •

edited

Loading

runspired Oct 22, 2016

pangratz Oct 22, 2016

stefanpenner Oct 22, 2016

runspired Oct 22, 2016

runspired commented Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016

pangratz Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016 •

edited

Loading

pangratz Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016

pangratz Oct 22, 2016

runspired Oct 22, 2016

pangratz Oct 23, 2016

runspired commented Oct 27, 2016

runspired commented Oct 28, 2016


		internalModel.loadingData(promise);
		let cache = this._pendingFetch.getWithDefault();

chore(store): cleans up and optimizes code paths around fetching rela… #4617

chore(store): cleans up and optimizes code paths around fetching rela… #4617

Conversation

runspired commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorT commented Oct 22, 2016

pangratz Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Oct 27, 2016

runspired commented Oct 28, 2016

pangratz Oct 22, 2016 •

edited

Loading

runspired Oct 22, 2016 •

edited

Loading