Upgraded IdentityMap and RecordMap #4664

runspired · 2016-11-17T22:18:42Z

When pushing records into the store (no materialization), we only needed to load the associated modelClass for "key" purposes. This PR shifts ember-data to consistently using a normalized modelName as the key, making it possible to push internal models, assemble record arrays, and (potentially in the future) even make requests without any access to the modelClass.

Step two of this effort (a separate PR) will be to enable the parsing of modelClass to avoid needing to load the classes for related models. Combined, this gives us a happy story for initial render in apps with pre-loaded data and complicated relationship graphs.

In addition, this paved an easier path forward for #4584

refactor typeMap => recordMap
refactor to using modelName instead of guid as the class identifier for recordMap.
refactor InternalModel to not need modelClass at instantiation.
remove calls to modelFor where now possible
remove typeMapFor
consider deprecation notices for InternalModel and buildInternalModel in case private APIs were abusing them and now need to refactor from modelClass to modelName.
(will be separate PR, but needed to complete this effort) refactor relationship setup to not need to lookup the related modelClass to prevent "pulling the string" on the entire graph.

UPDATES

I believe this is ready to ship but want to document a few things:

ClassMeta is not unit tested (yet) but it is implicitly tested as it exposes an identical interface to the pojo that was our TypeMap before.
@stefanpenner rightly points out areas we should be doing more to encapsulate things in this class, but this should be a follow up PR (that also moves to direct unit testing)
A temporary compromise has been made to give store to new ClassMeta(store) instead of owner in order to support the ability for it to resolve the modelClass via store.modelFor. Longer term this should be owner store.modelFor should instead use classMeta.modelClass to resolve the modelClass if necessary.
with this PR, _push does not require modelClass at all. The relationship PR is related to this "lazy push" story but is not required for it.

runspired · 2016-11-17T22:20:15Z

cc @stefanpenner got the first big part done, should have this PR done by EOD, hoping to at least have started the relationship PR that needs to accompany. Would like thoughts on the new location of ClassMeta, especially as I'm envisioning this as a spot to refactor what was my intended use of Schema.

runspired · 2016-11-17T22:24:16Z

addon/-private/system/class-meta.js

+      for (let i = 0; i < records.length; i++) {
+        record = records[i];
+        record.unloadRecord();
+        record.destroy(); // maybe within unloadRecord


I copied this over from the existing unloadAll functionality, but I suspect this is actually buggy an we want to reset the length of this._records to 0 here. I left it as is with the guess that unloadRecord removes it from the array on it's own (which would also be why we slice).

ya, I think (but we should double check with someone else) we want to set this._records.length = 0 just after let records = this._records.slice();

runspired · 2016-11-18T03:47:28Z

addon/-private/system/class-meta.js

+  }
+
+  /* deprecated way of accessing modelClass */
+  get type() {


we still use "type" enough places that I haven't pulled the trigger on an actual deprecation notice yet, want to keep this quieter until we're closer to the end of internals cleanup.

this may be a bit zalgo, as if there is no modelClass via type call, this will be null.

Wont the getter for modelClass lazily lookup the model class or is there something I'm missing?

@bmac I think the issue raised is that if there is (literally) no modelClass to resolve, that it will always be null, and ergo we will perpetually keep trying to resolve it with every "get" of type or modelClass.

stefanpenner · 2016-11-18T05:03:58Z

I am excited about this, but I would love to review this thoroughly, although I may not have cycles until tomorrow or sat. But will try for sooner

stefanpenner

I'm on-board with this direction, left some thoughts some of which should be addressed. Feel free to ping me if you want to discuss more.

stefanpenner · 2016-11-20T16:45:08Z

addon/-private/system/class-meta.js

+  }
+
+  destroy() {
+    this._store = null;


we should add the conventional isDestroying isDestroyed fields, so when debugging we can quickly see what is up.

stefanpenner · 2016-11-20T16:49:37Z

addon/-private/system/record-arrays/record-array.js

+      return null;
+    }
+    return this.store.modelFor(this.modelName);
+  }),


@stefanpenner making this change made our tests fail :'(

for good reasons, or test bugs?

This doesn't do what you think it is. (the last arg is dropped, but i suspect we want it to actually run)

I suspect you want:

type: computed('modelName', function() { // ... }).readOnly()

Indeed, wanted this.

stefanpenner · 2016-11-20T16:52:36Z

addon/-private/system/store.js


-    return typeMap;
+    recordMap = new ClassMeta(this, modelName);
+    recordMaps[modelName] = recordMap;


should this be an argument to ClassMeta ?

I originally was going to pass in owner, but wanted to piggy-pack on the modelFor behavior (for now). I think longer term modelFor would move into this object itself.

seems good.

stefanpenner · 2016-11-20T16:53:58Z

addon/-private/system/store.js

    let id = internalModel.id;

    internalModel.updateRecordArrays();

    if (id) {
-      delete typeMap.idToRecord[id];
+      delete recordMap.idToRecord[id];


should we add removeRecordById to classMeta / recordMap ?

stefanpenner · 2016-11-20T16:54:35Z

addon/-private/system/store.js

    }

-    let loc = typeMap.records.indexOf(internalModel);
-    typeMap.records.splice(loc, 1);
+    let loc = recordMap.records.indexOf(internalModel);


should we this also be pushed into classMeta / recordMap rather then exposing the internals?

stefanpenner · 2016-11-20T17:00:03Z

addon/-private/system/class-meta.js

+  }
+
+  get metadata() {
+    return this._metadata || (this._metadata = new EmptyObject());


this may also be like above

unlike above, the way this one works we don't deal with an "what if always resolves to null?" situation.

so its never inspected for empty-ness, always to append/mutate?

stefanpenner · 2016-11-20T17:00:46Z

addon/-private/system/class-meta.js

+  }
+
+  /* deprecated way of accessing modelClass */
+  get type() {


this may be a bit zalgo, as if there is no modelClass via type call, this will be null.

stefanpenner · 2016-11-20T17:01:42Z

addon/-private/system/class-meta.js

+      for (let i = 0; i < records.length; i++) {
+        record = records[i];
+        record.unloadRecord();
+        record.destroy(); // maybe within unloadRecord


ya, I think (but we should double check with someone else) we want to set this._records.length = 0 just after let records = this._records.slice();

stefanpenner · 2016-11-20T17:03:06Z

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

@@ -104,13 +104,12 @@ const {
  @class InternalModel
 */
 export default class InternalModel {
-  constructor(modelClass, id, store, data) {
+  constructor(modelName, id, store, data) {
    heimdall.increment(new_InternalModel);


should the internalModel have a direct reference to its classMeta / internalClass, that way get modelClass() can simply delegate to it.

I agree but wanted to limit the scope of the initial PR

sounds reasonable

stefanpenner · 2016-11-20T17:04:56Z

addon/-private/system/store.js

-  getReference: function(type, id) {
-    return this._internalModelForId(type, id).recordReference;
+  getReference(modelName, id) {
+    modelName = this._classKeyFor(modelName);


override argument value makes debugging annoying

Agreed, hadn't thought about that, will refactor to use a new variable name. I'd also like to bring up (as a separate PR) that it would be nice to officially deprecate support for camelCase modelName usage.

seems fine, as long as we don't forget

stefanpenner · 2016-11-20T17:07:31Z

@runspired given this PR, what remains re: loading modelClass spuriously during cache load?

stefanpenner · 2016-11-20T17:18:52Z

@emberjs/ember-data-contributors we would love more sets of eyes on this, we don't want to unilaterally sneak something like this in.

bmac · 2016-11-21T15:12:11Z

I'm 👍 on the direction of this change.

runspired · 2016-11-21T19:26:18Z

@stefanpenner with these changes, you no longer load a modelClass for anything not related to

iterating attributes / relationships
adapter calls
serializer calls

Iteration currently only happens during create/update/delete data flows, so this means that for what we wanted as a first pass this achieves that.

stefanpenner · 2016-11-21T19:32:34Z

@runspired nice, have you tested this in our app. I want to make sure we didn't forget anything obviously for those offending code-paths.

workmanw · 2016-11-21T20:19:00Z

Not surprisingly, this does seem to break some ember-data-model-fragment tests. I'll look into getting that fixed once this lands. One question though, you said there is also a coming PR for part two of this effort. Do you plan to squeeze that into the same ember-data release? If so, I'll wait to investigate until the effort is complete.

Edit: There are also some other breaking tests that seem related to serializer changes for model-fragments. Just looking for the dust to settle before I dive in.

stefanpenner · 2016-11-21T21:06:12Z

@workmanw is there an effort to get model-fragments using public APIs and/or derive the needed public API's for model-fragments to be successful? We would love to not cause you extra work, but the intimacy between the two projects appears quite fragile.

workmanw · 2016-11-27T02:39:38Z

@stefanpenner Sorry I missed your response. To be perfectly honest, I wouldn't even know where to begin. I don't have a commanding knowledge of the ember-data internals. I'm not the only maintainer nor the original author, so I can't speak for everyone, but I would be open to trying to make this more durable.

The biggest challenges we have in ember-data-model-fragments are 1) we interact heavily with the internal model and 2) we have to monkey patch part of the ember-data public API. For example, we override DS.Model.rollbackAttributes, DS.Model. createSnapshot, DS.model.adapterDidCommit, etc. Most of the monkey patching is just trying to keep the state of the model and it's fragments in sync.

If you have idea's on how ember-data's API could be evolved to help facilitate this, I'd love to hear them.

stefanpenner · 2016-11-27T03:26:30Z

@workmanw a good start is thoroughly documenting all the things that require private API usage (and why). That way those more familiar with ember-data can help with what would need to change.

stefanpenner · 2016-12-03T09:49:46Z

@runspired let me know when/if you ready for another code-review

runspired · 2016-12-09T04:41:54Z

addon/-private/system/record-map.js

+
+  /* deprecated way of accessing modelClass */
+  get type() {
+    throw new Error('RecordMap.type is no longer available');


historically we've had type stashed away, but @igorT was skeptical it should have been there. It is "nice to have" but we should think it through more carefully. I added this error just to see if anything in the codebase depended on it being there, it doesn't appear that anything did!

stefanpenner · 2016-12-12T02:00:47Z

addon/-private/system/identity-map.js

+   @method clear
+   */
+  clear() {
+    let recordMaps = this._map;


Do we need to actually clear these, our should we just "release" the dominator?

I believe we must specifically clear them because clear is responsible for tearing down the internal models. This is a refactor that preserves existing logic for teardown, but it is absolutely possible we were doing an excessive amount of work on teardown in the past.

ah ok, lets punt.

@stefanpenner all but 2 tests still pass with use simply releasing, and they look to be two tests that are testing whether internal-model teardown things are done.

stefanpenner · 2016-12-12T02:02:37Z

addon/-private/system/record-arrays/record-array.js

+      return null;
+    }
+    return this.store.modelFor(this.modelName);
+  }),


This doesn't do what you think it is. (the last arg is dropped, but i suspect we want it to actually run)

I suspect you want:

type: computed('modelName', function() { // ... }).readOnly()

stefanpenner · 2016-12-12T02:12:37Z

addon/-private/system/identity-map.js

+   @method clear
+   */
+  clear() {
+    let recordMaps = this._map;


ah ok, lets punt.

bmac · 2016-12-12T15:48:21Z

This looks good to me @runspired. Do you mind rebasing it when you have a chance?

… of guid as the class identifier for recordMap

runspired · 2016-12-12T17:05:20Z

@bmac rebased!

runspired commented Nov 17, 2016

View reviewed changes

runspired changed the title ~~[WIP] A faster and lazier store._push story.~~ A faster and lazier store._push story. Nov 18, 2016

runspired commented Nov 18, 2016

View reviewed changes

runspired mentioned this pull request Nov 18, 2016

[WIP] [Feat] implements Schema to separate store logic and model description from the model layer. #4584

Closed

7 tasks

stefanpenner suggested changes Nov 20, 2016

View reviewed changes

runspired mentioned this pull request Nov 30, 2016

pushPayload/save can create duplicate records in the store #4262

Closed

runspired force-pushed the feat/silent-push branch from c3dbc42 to c54e7a1 Compare November 30, 2016 18:44

runspired changed the title ~~A faster and lazier store._push story.~~ Upgraded IdentityMap and RecordMap Dec 9, 2016

runspired force-pushed the feat/silent-push branch from aa86e09 to df21a7b Compare December 9, 2016 03:19

runspired commented Dec 9, 2016

View reviewed changes

runspired mentioned this pull request Dec 9, 2016

Factory cache #4704

Merged

runspired force-pushed the feat/silent-push branch 2 times, most recently from a89c7ee to 22971f9 Compare December 12, 2016 01:24

stefanpenner suggested changes Dec 12, 2016

View reviewed changes

stefanpenner approved these changes Dec 12, 2016

View reviewed changes

addon/-private/system/identity-map.js

@method clear

*/

clear() {

let recordMaps = this._map;

Copy link

Member

stefanpenner Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, lets punt.

bmac approved these changes Dec 12, 2016

View reviewed changes

refactor typeMap => recordMap and refactor to using modelName instead…

a5a8cd7

… of guid as the class identifier for recordMap

runspired added 2 commits December 12, 2016 09:04

fix test

eaa3435

fix readOnly

fe74d89

runspired force-pushed the feat/silent-push branch from 937d241 to fe74d89 Compare December 12, 2016 17:05

bmac merged commit 63d725d into emberjs:master Dec 13, 2016

Upgraded IdentityMap and RecordMap #4664

Upgraded IdentityMap and RecordMap #4664

Conversation

runspired commented Nov 17, 2016 • edited Loading

runspired commented Nov 17, 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

stefanpenner commented Nov 18, 2016 • edited Loading

stefanpenner left a 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

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

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

stefanpenner commented Nov 20, 2016

stefanpenner commented Nov 20, 2016

bmac commented Nov 21, 2016

runspired commented Nov 21, 2016

stefanpenner commented Nov 21, 2016

workmanw commented Nov 21, 2016 • edited Loading

stefanpenner commented Nov 21, 2016 • edited Loading

workmanw commented Nov 27, 2016

stefanpenner commented Nov 27, 2016 • edited Loading

stefanpenner commented Dec 3, 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

Choose a reason for hiding this comment

bmac commented Dec 12, 2016

runspired commented Dec 12, 2016

runspired commented Nov 17, 2016 •

edited

Loading

stefanpenner commented Nov 18, 2016 •

edited

Loading

workmanw commented Nov 21, 2016 •

edited

Loading

stefanpenner commented Nov 21, 2016 •

edited

Loading

stefanpenner commented Nov 27, 2016 •

edited

Loading