[CHORE]: Extract internalModel access to identifiers in RecordArray modules #6715

snewcomer · 2019-11-11T05:57:01Z

This is to reduce the need of InternalModel throughout the codebase. This is possible because we have an identifier cache available to lookup records that were populated locally. This doesn't encompass all the spots where an identifier can be swapped for an InternalModel, but is a start.

Modifications include Record-Array, Record-Array-Manager, Adapter-Populated-Record-Array and Core-Store.

Method calls on RecordArray give identifiers instead of internalModels
Calls to Record-Array-Manager pass identifiers and expects to receive identifiers.

ref #6166

packages/-ember-data/tests/integration/record-arrays/adapter-populated-record-array-test.js

snewcomer · 2019-11-11T06:04:02Z

packages/-ember-data/tests/integration/relationships/has-many-test.js

@@ -3679,75 +3679,70 @@ module('integration/relationships/has_many - Has-Many Relationships', function(h
      user: belongsTo('user', { inverse: null, async: false }),
    });

-    run(() => {


just reminding to revisit this...

snewcomer · 2019-11-11T06:05:47Z

packages/store/addon/-private/system/model/internal-model.ts

@@ -122,7 +122,6 @@ export default class InternalModel {
  __recordData: RecordData | null;
  _isDestroyed: boolean;
  isError: boolean;
-  _pendingRecordArrayManagerFlush: boolean;


this state was only used in record-array-manager, so put it there with a Set of pending identifiers.

snewcomer · 2019-11-11T06:06:53Z

packages/store/addon/-private/system/record-array-manager.js

+
+function recordArraysForIdentifier(store, identifier) {
+  let internalModel = internalModelFactoryFor(store).peek(identifier);
+  if (internalModel) {


There was only one failing test that required this guard. disconnectRecord in store-wrapper-test. It is pushing with data: [] and included: [houseHash]. Is this possible?

Turns out M3 hit this case so I had to make some adjustments, roughly speaking it is possible in the current state of things to try to sync-destroy during push via this API. I'm going to bring this up in a meeting as EmberData does not do a sync-destroy here itself and we likely should not allow it going forward as the "syncLiveArray" code is undesirable complexity that remains from our need to handle observers greedily firing when we begin changes.

We may be able to refactor overtime to avoid delivering changes to the UI until a single "ui flush" which would help us avoid this problem.

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

runspired · 2019-11-12T17:34:03Z

The M3 failure is because M3 registers query-arrays (result of M3's queryURL method) with the record-array-manager. There's a few paths forward here:

Intercept Approach
- detect internalModels during creation and convert them
- implement legacy _pushIM/_setIM methods and convert to identifiers within
Refactor M3
- for the non-custom-model-class branch, move M3 to using identifiers or off of RecordArrayManager
Wait for M3
- once the custom-model-class FF is on, M3 no longer uses the RecordArrayManager

igorT · 2019-11-15T22:56:33Z

packages/-ember-data/tests/integration/record-array-manager-test.js

@@ -100,7 +100,7 @@ module('integration/record_array_manager', function(hooks) {
    assert.equal(allSummary.called.length, 0, 'initial: no calls to all.willDestroy');
    assert.equal(adapterPopulatedSummary.called.length, 0, 'initial: no calls to adapterPopulated.willDestroy');
    assert.equal(
-      internalPersonModel._recordArrays.size,
+      manager.getRecordArrayForIdentifier(internalPersonModel.identifier).size,


this method should be getRecordArraysForIdentifier

igorT · 2019-11-15T22:57:43Z

packages/-ember-data/tests/integration/record-array-manager-test.js

@@ -100,7 +100,7 @@ module('integration/record_array_manager', function(hooks) {
    assert.equal(allSummary.called.length, 0, 'initial: no calls to all.willDestroy');
    assert.equal(adapterPopulatedSummary.called.length, 0, 'initial: no calls to adapterPopulated.willDestroy');
    assert.equal(
-      internalPersonModel._recordArrays.size,
+      manager.getRecordArrayForIdentifier(internalPersonModel.identifier).size,


me and @runspired talked about adding a test to confirm that getRecordArrayForIdentifier is === _recordArrays

packages/store/addon/-private/system/model/internal-model.ts

igorT · 2019-11-15T23:09:55Z

packages/store/addon/-private/system/record-array-manager.js


 const emberRun = emberRunloop.backburner;
+const pendingForIdentifier = new Set([]);
+const IMFallback = new WeakMap();
+const RecordArraysCache = new WeakMap(); // store StableIdentifier => Set[RecordArray[]]


Can we move the comment above the line please

runspired · 2019-11-16T01:02:02Z

After reviewing with @igorT we decided we will

introduce a feature flag for RECORD_ARRAY_IDENTIFIERS, when off this PR should in effect have "no change"
introduce a feature flag for DEPRECATED_PRIVATE_RECORD_ARRAY_SUPPORT, which when RECORD_ARRAY_IDENTIFIERS is on and DEPRECATED_PRIVATE_RECORD_ARRAY_SUPPORT is off has the effect of removing our legacy support for private API hacks used to add custom record arrays (such as what M3 does).
update this PR to make use of these flags
add some tests for private API hacks that use the record array manager
land this PR then land a PR turning both the new flags on.
at a later time (likely around the time that CUSTOM_MODEL_CLASS flag is activated) turn the DEPRECATED_PRIVATE_RECORD_ARRAY_SUPPORT off.

snewcomer · 2020-02-03T02:03:19Z

packages/store/addon/-private/system/model/internal-model.ts

@@ -157,6 +157,7 @@ export default class InternalModel {
  _scheduledDestroy: any;
  _modelClass: any;
  __deferredTriggers: any;
+  __recordArrays: any;


I brought this back b/c we use __recordArrays in recordArray._dissociateFromOwnRecords.

Since internal-model has an identifier regardless of the flag, I think we would be safe to refactor.

runspired · 2020-02-05T01:06:12Z

packages/-ember-data/tests/integration/record-arrays/adapter-populated-record-array-test.js

@@ -18,7 +19,7 @@ const Person = Model.extend({
  },
 });

-module('integration/record-arrays/adapter_populated_record_array - AdapterPopulatedRecordArray', function(hooks) {
+module('scott integration/record-arrays/adapter_populated_record_array - AdapterPopulatedRecordArray', function(hooks) {


don't forget the Scott removal

snewcomer · 2020-02-22T21:39:29Z

@runspired I believe this PR is ready to review. All tests passed with both feature flags on and off!!

igorT · 2020-07-16T23:16:38Z

packages/canary-features/addon/default-features.ts

@@ -177,4 +177,6 @@ export default {
  REQUEST_SERVICE: null,
  CUSTOM_MODEL_CLASS: null,
  FULL_LINKS_ON_RELATIONSHIPS: true,
+  RECORD_ARRAY_MANAGER_IDENTIFIERS: false,


These should be null

igorT · 2020-07-16T23:29:24Z

packages/store/addon/-private/system/record-array-manager.js

-    array._removeInternalModels(modelsToRemove);
-  }
-}
+  const pushIdentifiers = function(recordArray, identifiers, cache) {


Can we name this function please

igorT · 2020-07-16T23:37:33Z

packages/store/addon/-private/system/record-array-manager.js

 const emberRun = emberRunloop.backburner;
+let RecordArrayManager;
+
+export let associateWithRecordArray;


Lets export this after it's been assigned

Since we have this assigned in a conditional block, it only allows us to import/export at the top level.

igorT · 2020-07-16T23:49:43Z

packages/store/addon/-private/system/record-array-manager.js

+    }
+  };
+
+  const destroy = function(entry) {


Lets bring these back into the named style, and see if we run into issues

igorT · 2020-07-23T21:01:55Z

packages/store/addon/-private/system/core-store.ts

@@ -2657,7 +2658,12 @@ abstract class CoreStore extends Service {
    internalModel.setupData(data);

    if (!isUpdate) {
-      this.recordArrayManager.recordDidChange(internalModel);
+      // TODO: do we want to maintain old API with this flag?


This seems like a leftover comment

igorT · 2020-07-23T21:03:26Z

packages/store/addon/-private/system/core-store.ts

-      this.recordArrayManager.recordDidChange(internalModel);
+      // TODO: do we want to maintain old API with this flag?
+      if (RECORD_ARRAY_MANAGER_IDENTIFIERS) {
+        this.recordArrayManager.recordDidChange(internalModel.identifier);


Given that we use the identifier on line 2644, we could extract the let identifier

igorT · 2020-07-23T21:07:01Z

packages/store/addon/-private/system/internal-model-map.ts

@@ -42,6 +44,10 @@ export default class InternalModelMap {
    return this._models.length;
  }

+  get modelIdentifiers(): StableRecordIdentifier[] {


we should probably call this recordIdentifiers. We also only use it in a single place, maybe it's not worth creating a method for it?

igorT · 2020-07-23T21:08:11Z

packages/store/addon/-private/system/model/internal-model.ts

@@ -1363,8 +1359,14 @@ export default class InternalModel {
    @private
  */
  updateRecordArrays() {
-    // @ts-ignore: Store is untyped and typescript does not detect instance props set in `init`
-    this.store.recordArrayManager.recordDidChange(this);
+    // TODO: do we want to maintain old API with this flag?


Leftover comment

igorT · 2020-07-23T21:11:59Z

packages/store/addon/-private/system/model/internal-model.ts

-    this.store.recordArrayManager.recordDidChange(this);
+    // TODO: do we want to maintain old API with this flag?
+    if (RECORD_ARRAY_MANAGER_IDENTIFIERS) {
+      // @ts-ignore: Store is untyped and typescript does not detect instance props set in `init`


This ts-ignore is slightly incorrect, it seems we just need to expose recordArrayManager from CoreStore

igorT · 2020-07-23T21:15:12Z

packages/store/addon/-private/system/record-array-manager.js

+  REMOVE_RECORD_ARRAY_MANAGER_LEGACY_COMPAT,
+} from '@ember-data/canary-features';
+
+// only used by RECORD_ARRAY_MANAGER_IDENTIFIERS


Lets remove this comment

igorT · 2020-07-23T21:16:45Z

packages/store/addon/-private/system/record-array-manager.js

+
+    /**
+     * @method recordDidChange
+     * @public


I don't think this is meant to be public, no?

igorT · 2020-07-23T21:17:09Z

packages/store/addon/-private/system/record-array-manager.js

+      @param {String} modelName
+      @param {Object} query
+      @param {Array} internalModels
+      @param {Object} payload


igorT · 2020-07-23T21:19:31Z

packages/store/addon/-private/system/record-array-manager.js

+    }
+  };
+
+  const destroy = function destroyEntry(entry) {


should make the names consistent

igorT · 2020-07-23T21:19:36Z

packages/store/addon/-private/system/record-array-manager.js

+    entry.destroy();
+  };
+
+  const remove = function removeEntry(array, item) {


should make the names consistent

igorT · 2020-07-23T21:25:54Z

packages/store/addon/-private/system/record-arrays/adapter-populated-record-array.js

+    @param {Object} payload normalized payload
+    @internal
+  */
+  AdapterPopulatedRecordArray.prototype._setIdentifiers = function _setIdentifiers(identifiers, payload) {


Seems safer to extend here

Done! I think the original intention was to work on the object directly instead of subclassing but this works too! Aligns closer to the semantics of defining objects in this file as well.

One question though - When it comes to switching this file over to TS though, would we modify the prototype or subclass?

igorT · 2020-07-23T21:26:37Z

packages/store/addon/-private/system/record-arrays/adapter-populated-record-array.js

+    this.manager._associateWithRecordArray(identifiers, this);
+
+    if (DEPRECATE_EVENTED_API_USAGE) {
+      const _hasDidLoad = DEBUG ? this._has('didLoad') : this.has('didLoad');


Switch with let

igorT · 2020-07-23T21:33:24Z

packages/store/addon/-private/system/record-arrays/adapter-populated-record-array.js

+    @param {Object} payload normalized payload
+    @internal
+  */
+  AdapterPopulatedRecordArray.prototype._setIdentifiers = function _setIdentifiers(identifiers, payload) {


igorT · 2020-07-23T21:37:02Z

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

+  return internalModelForIdentifier(store, identifier).getRecord();
+}
+
+function internalModelForIdentifier(store, identifier) {


We probably don't need this helper function

igorT · 2020-07-23T21:38:08Z

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

@@ -113,8 +124,13 @@ export default ArrayProxy.extend(DeprecatedEvented, {
    @return {Model} record
  */
  objectAtContent(index) {
-    let internalModel = get(this, 'content').objectAt(index);
-    return internalModel && internalModel.getRecord();
+    let identifier = get(this, 'content').objectAt(index);


this might read nicer if

if (RECORD_ARRAY_MANAGER_IDENTIFIERS) { let identifier = get(this, 'content').objectAt(index); return identifier ? recordForIdentifier(this.store, identifier) : undefined; } else { let internalModel = get(this, 'content').objectAt(index); return internalModel ? internalModel.getRecord() : undefined; }

igorT · 2020-07-23T21:39:06Z

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


-  /*
+if (RECORD_ARRAY_MANAGER_IDENTIFIERS) {


Should probably be extending here

igorT · 2020-07-23T21:40:36Z

packages/unpublished-fastboot-test-app/app/templates/application.hbs

@@ -2,4 +2,4 @@

 {{outlet}}

-<a href="/tests">Tests</a>
+<a href={{"/tests"}}>Tests</a>


Lets try removing this

igorT · 2020-07-23T21:53:09Z

packages/store/addon/-private/system/record-array-manager.js

 const emberRun = emberRunloop.backburner;
+let RecordArrayManager;
+
+export let associateWithRecordArray;


Lets leave a comment that we should remove this after the FF land

…odules

snewcomer commented Nov 11, 2019

View reviewed changes

runspired added 🏷️ chore This PR primarily refactors code or updates dependencies 🎯 canary PR is targeting canary (default) 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166 labels Nov 11, 2019

runspired added the blocked label Nov 12, 2019

runspired force-pushed the sn/record-arrays-next branch 2 times, most recently from 5ceb06e to 7d8e6de Compare November 14, 2019 20:30

runspired removed the blocked label Nov 14, 2019

igorT reviewed Nov 15, 2019

View reviewed changes

runspired force-pushed the sn/record-arrays-next branch from c6ab05d to ad7fd41 Compare December 12, 2019 23:50

snewcomer mentioned this pull request Jan 20, 2020

[CHORE]: Extract internalModel access to identifiers in RecordArray modules #6981

Closed

runspired force-pushed the sn/record-arrays-next branch from bcee390 to c5968e2 Compare February 1, 2020 00:37

snewcomer commented Feb 3, 2020

View reviewed changes

runspired reviewed Feb 5, 2020

View reviewed changes

snewcomer force-pushed the sn/record-arrays-next branch from 62c38ab to 2020758 Compare February 22, 2020 19:25

snewcomer force-pushed the sn/record-arrays-next branch from 975f75c to eb97486 Compare March 3, 2020 04:13

igorT assigned runspired and igorT Apr 1, 2020

snewcomer force-pushed the sn/record-arrays-next branch from eb97486 to 44772f2 Compare April 22, 2020 21:03

runspired removed the 🎯 canary PR is targeting canary (default) label Apr 25, 2020

snewcomer force-pushed the sn/record-arrays-next branch from 44772f2 to 6b8e07b Compare April 27, 2020 02:46

igorT reviewed Jul 16, 2020

View reviewed changes

snewcomer added the code-review Tracks PRs that require a code-review label Jul 21, 2020

igorT reviewed Jul 23, 2020

View reviewed changes

snewcomer requested a review from igorT July 24, 2020 22:08

snewcomer force-pushed the sn/record-arrays-next branch from 6a1f0f7 to 940efc7 Compare July 29, 2020 22:25

[CHORE]: Extract internalModel access to identifiers in RecordArray m…

65a8ac4

…odules

snewcomer force-pushed the sn/record-arrays-next branch from 940efc7 to 65a8ac4 Compare July 30, 2020 01:14

igorT merged commit a3fd90f into emberjs:master Jul 30, 2020

snewcomer deleted the sn/record-arrays-next branch July 30, 2020 02:55

snewcomer mentioned this pull request Aug 2, 2020

[CHORE]: Extract internalModel access to identifiers for Relationships #7262

Merged

locks mentioned this pull request Oct 6, 2020

Created an announcement of Ember 3.22 release ember-learn/ember-blog#785

Merged

snewcomer mentioned this pull request Oct 14, 2020

[Feature Flag]: RECORD_ARRAY_MANAGER_IDENTIFIERS: true #7353

Merged

dependabot bot mentioned this pull request Mar 15, 2021

chore(deps-dev): bump ember-data from 3.17.0 to 3.25.0 movableink/ember-data-json-api-bulk-ext#157

Closed

[CHORE]: Extract internalModel access to identifiers in RecordArray modules #6715

[CHORE]: Extract internalModel access to identifiers in RecordArray modules #6715

Conversation

snewcomer commented Nov 11, 2019 • 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

runspired commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Nov 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Feb 22, 2020

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

igorT Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

igorT Jul 23, 2020 • 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

igorT Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorT Jul 23, 2020 • 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

snewcomer commented Nov 11, 2019 •

edited

Loading

igorT Jul 23, 2020 •

edited

Loading

igorT Jul 23, 2020 •

edited

Loading

igorT Jul 23, 2020 •

edited

Loading

igorT Jul 23, 2020 •

edited

Loading