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

runspired · 2016-10-18T07:20:16Z

TODO

Refactor areas where internal model is being used as schema
Move as much logic off of DS.Model methods as we can, since it's public API we'll need to continue exposing it on that class, just not use it ourselves.
Refactor InternalModel down to just a data/state container
? Refactor Model methods to use schema for descriptor iteration (tricky because Map => EmptyObject ) ?
? consider refactoring out relationshipMap, as it's almost certain to be greedy and allocates a bunch of arrays. While this may be better if we often iterate over relationships by related className, I suspect such iteration is rare ?
? InternalModel._loadingPromise ?
? debugSeal on InternalModel so we can find anywhere we're being bad currently ?

tchak · 2016-10-18T09:50:12Z

addon/-private/system/store.js

-    var id = coerceId(inputId);
-    var internalModel = this.typeMapFor(typeClass).idToRecord[id];
+    let id = coerceId(inputId);
+    let internalModel = this.schemaFor(modelName).recordMap.idToRecord[id];


a few lines bellow you saying recordMap.idToRecord[id] holds records

tchak · 2016-10-18T09:50:52Z

addon/-private/system/store.js

-    var record = idToRecord[id];
+    let id = coerceId(inputId);
+    let schema = this.schemaFor(modelName);
+    let record = schema.recordMap.idToRecord[id];


a few lines up you saying recordMap.idToRecord[id] holds internalModels

this should be internalModel, I was carrying over the old names and should have noticed the old one was bad

tchak · 2016-10-18T09:57:52Z

addon/-private/system/schema.js

+        idToRecord: new EmptyObject(),
+        records: [],
+        metadata: new EmptyObject(),
+        type: this.model


what is this.model?

It's nothing, it should be this.modelClass

Are you keeping type here and bellow because it would be too much work for now to refactor everything?

@tchak more or less yes, I actually went down the road of refactoring everything in an earlier spike, but the problem is that a few of these places leak to public code and I'm no sure if we consider this one to yet or not.

tchak · 2016-10-18T21:55:22Z

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


-  referenceFor(type, name) {
+  referenceFor(modelClass, name) {


here type is referring to relationship kind : hasMany or belongsTo

whoops, will change that back. I think maybe we should consistently refer to this as kind internally? I noticed this confusion in another spot too.

👍 for using kind instead of type

runspired · 2016-10-18T22:14:21Z

Early results for this branch have it at roughly (4ms / 238 records of 1 type over the 28ms of the internal-model branch it branches off of (this branch does not include the cache upgrades currently). This is a happy surprise, because most of the perf improvements this unlocks have not been worked on yet.

fivetanley · 2016-10-19T05:05:37Z

will give a second review tomorrow, but i am crying tears of joy right now.

pangratz · 2016-10-19T09:09:44Z

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

+
+  get type() {
+    deprecate(`InternalModel.type has been deprecated in favor of InternalModel.modelClass`, {
+      id: 'ember-data.private.type-to-modelClass'


needs an until: "3.0"

pangratz · 2016-10-19T09:16:18Z

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


-  referenceFor(type, name) {
+  referenceFor(modelClass, name) {


👍 for using kind instead of type

pangratz · 2016-10-19T09:18:25Z

addon/-private/system/schema.js

+    }
+    if (!this._serializer) {
+      throw new Error('Cant find serializer!');
+    }


Is this a leftover from refactoring? Do we need this check for an serializer, since there should always be a default serializer, no?

I believe it's just a left over check for something that was failing for me early in the process and can be removed

pangratz · 2016-10-19T09:20:40Z

addon/-private/system/schema.js

+      inverse = inverseSchema.relationships[inverseName];
+
+      assert("We found no inverse relationships by the name of '" + inverseName + "' on the '" + inverseSchema.modelName +
+        "' model. This is most likely due to a missing attribute on your model definition.", !Ember.isNone(inverse));


s/attribute/relationship/

pangratz · 2016-10-19T09:38:34Z

addon/-private/system/store.js

+
+    if (this._schemas === null) {
+      this._schemas = new EmptyObject();
+    }


I think it makes sense to initialize this._schemas in the init.

pangratz · 2016-10-19T09:45:21Z

addon/-private/system/store.js

  */
-  typeMapFor(typeClass) {
+  typeMapFor(modelClass) {


Should this be renamed to recordMapFor to better reflect what is returned?

@pangratz yes, but I suspect we leaked this publicly and should add a deprecation.

runspired · 2016-11-18T08:13:41Z

I rebased this, but I I want to shrink it's scope a bit and reconsider things. I should be able to land this by end of day tomorrow by branching off of #4664 and moving parts of the schema into ClassMeta instead and discarding much of the rest.

I think this implementation at this point in time is too broad, because we will likely want to expose a primitive (not necessarily exactly this one) to adapters in place of modelClass, but this one is not ready for that. I have a plan, but it's happily more incremental, and will turn into multiple simpler PRs instead of one larger one.

runspired · 2016-12-09T07:03:58Z

As luck would have it, once I started trying to port some of the iterator / model parse optimizations into DS.Model, turns out we still need this! So nvm on the new direction, cleaning up this PR and shifting it's focus a wee bit to be an object tied more closely to DS.Model for now.

chore(modelClass): converts InternalModel.type to InternalModel.modelClass houston, we have a schema move typeMap into schema port typeMapFor usage to schemaFor().recordMap type/typeClass => modelClass, type => modelName

runspired · 2017-03-16T00:36:03Z

Closing in favor of #4865

runspired force-pushed the feat/schema branch 2 times, most recently from 2396e4e to 1549c26 Compare October 18, 2016 18:37

tchak reviewed Oct 18, 2016

View reviewed changes

pangratz reviewed Oct 19, 2016

View reviewed changes

runspired force-pushed the feat/schema branch from 1549c26 to 52ebb7f Compare October 19, 2016 12:14

runspired force-pushed the feat/schema branch from 52ebb7f to bfa9184 Compare November 17, 2016 05:08

runspired mentioned this pull request Nov 18, 2016

Upgraded IdentityMap and RecordMap #4664

Merged

6 tasks

runspired added 2 commits December 8, 2016 23:15

feat(schema): adds Schema and store.schemaFor

434ba3c

chore(modelClass): converts InternalModel.type to InternalModel.modelClass houston, we have a schema move typeMap into schema port typeMapFor usage to schemaFor().recordMap type/typeClass => modelClass, type => modelName

gets tests passing

f22850a

runspired force-pushed the feat/schema branch from bfa9184 to f22850a Compare December 9, 2016 07:16

sly7-7 mentioned this pull request Dec 14, 2016

remove usage and deprecate InternalModel's type property #4717

Closed

runspired closed this Mar 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

runspired commented Oct 18, 2016 •

edited

Loading

tchak Oct 18, 2016

tchak Oct 18, 2016

runspired Oct 18, 2016

tchak Oct 18, 2016

runspired Oct 18, 2016

tchak Oct 18, 2016

runspired Oct 18, 2016

tchak Oct 18, 2016

runspired Oct 18, 2016

pangratz Oct 19, 2016

runspired commented Oct 18, 2016

fivetanley commented Oct 19, 2016

pangratz Oct 19, 2016

pangratz Oct 19, 2016

pangratz Oct 19, 2016

runspired Oct 19, 2016

pangratz Oct 19, 2016

pangratz Oct 19, 2016

pangratz Oct 19, 2016

runspired Oct 19, 2016

runspired commented Nov 18, 2016

runspired commented Dec 9, 2016

runspired commented Mar 16, 2017

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

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

Conversation

runspired commented Oct 18, 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

runspired commented Oct 18, 2016

fivetanley commented Oct 19, 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

Choose a reason for hiding this comment

runspired commented Nov 18, 2016

runspired commented Dec 9, 2016

runspired commented Mar 16, 2017

runspired commented Oct 18, 2016 •

edited

Loading