From d2909c4b4d959ce26d3cc79f03a5c9de042554cd Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Wed, 3 Feb 2021 16:20:34 -0800 Subject: [PATCH] [CLEANUP] Refactor DataAdapter to not use observers or array observers The DataAdapter class is exposed specifically for the Ember Inspector to support Ember Data. Currently, the adapter relies on both array observers and standard observers. This PR refactors the adapter to use autotracking-based methods for tracking changes instead, with minimal API churn. The main change is to introduce a regular poll that checks for changes after each backburner runloop. There are two types of polling done: 1. `TypeWatcher`, which checks to see if a record array has changed at all (e.g. contents have changed) 2. `RecordsWatcher`, which watches the array as a whole _and_ the individual records within it. Whenever any changes occur, it reiterates the records array, and looks for new records, updated records, and removed records, and notifies of any changes. This removes the need for both array observers _and_ standard observers. Standard observers were previously used to detect changes on the model instances, but now we autotrack _serializing_ them, and that autotracking is used to detect if they've been updated. \## Breaking Changes - The `recordsRemoved` callback from `watchRecords` has changed. It now receives an array of removed records, _not_ an index and count. On the consuming side in Ember Inspector, we'll have to update the code to remove the items passed via id explicitly. - The `observeRecord` method has been removed. Because serializing the object _is_ observing it (via autotracking) there is no need to manually observe it. Users who are still implementing this method (e.g. Ember Data) can still keep it, and it shouldn't cause issues, it just will no longer be called. --- .../extension-support/lib/data_adapter.js | 272 +++++++++++++----- .../tests/data_adapter_test.js | 51 +--- tests/docs/expected.js | 52 ++-- 3 files changed, 234 insertions(+), 141 deletions(-) diff --git a/packages/@ember/-internals/extension-support/lib/data_adapter.js b/packages/@ember/-internals/extension-support/lib/data_adapter.js index eedb6e92d4b..2c70eb722bd 100644 --- a/packages/@ember/-internals/extension-support/lib/data_adapter.js +++ b/packages/@ember/-internals/extension-support/lib/data_adapter.js @@ -1,8 +1,122 @@ import { getOwner } from '@ember/-internals/owner'; -import { scheduleOnce } from '@ember/runloop'; -import { get, objectAt, addArrayObserver, removeArrayObserver } from '@ember/-internals/metal'; +import { backburner } from '@ember/runloop'; +import { get } from '@ember/-internals/metal'; import { dasherize } from '@ember/string'; import { Namespace, Object as EmberObject, A as emberA } from '@ember/-internals/runtime'; +import { consumeTag, createCache, getValue, tagFor, untrack } from '@glimmer/validator'; + +function iterate(arr, fn) { + if (typeof Symbol !== 'undefined' && Symbol.iterator in arr) { + for (let item of arr) { + fn(item); + } + } else { + arr.forEach(fn); + } +} + +class RecordsWatcher { + recordCaches = new Map(); + + added = []; + updated = []; + removed = []; + + getCacheForItem(record) { + let recordCache = this.recordCaches.get(record); + + if (!recordCache) { + let hasBeenAdded = false; + + recordCache = createCache(() => { + if (!hasBeenAdded) { + this.added.push(this.wrapRecord(record)); + hasBeenAdded = true; + } else { + this.updated.push(this.wrapRecord(record)); + } + }); + + this.recordCaches.set(record, recordCache); + } + + return recordCache; + } + + constructor(records, recordsAdded, recordsUpdated, recordsRemoved, wrapRecord, release) { + this.release = release; + this.wrapRecord = wrapRecord; + + this.recordArrayCache = createCache(() => { + let seen = new Set(); + + // Track `[]` for legacy support + consumeTag(tagFor(records, '[]')); + + iterate(records, (record) => { + getValue(this.getCacheForItem(record)); + seen.add(record); + }); + + // Untrack this operation because these records are being removed, they + // should not be polled again in the future + untrack(() => { + this.recordCaches.forEach((cache, record) => { + if (!seen.has(record)) { + this.removed.push(wrapRecord(record)); + this.recordCaches.delete(record); + } + }); + }); + + if (this.added.length > 0) { + recordsAdded(this.added); + this.added = []; + } + + if (this.updated.length > 0) { + recordsUpdated(this.updated); + this.updated = []; + } + + if (this.removed.length > 0) { + recordsRemoved(this.removed); + this.removed = []; + } + }); + } + + revalidate() { + getValue(this.recordArrayCache); + } +} + +class TypeWatcher { + constructor(records, onChange, release) { + let hasBeenAccessed = false; + + this.cache = createCache(() => { + // Empty iteration, we're doing this just + // to track changes to the records array + iterate(records, () => {}); + + // Also track `[]` for legacy support + consumeTag(tagFor(records, '[]')); + + if (hasBeenAccessed === true) { + onChange(); + } else { + hasBeenAccessed = true; + } + }); + + this.release = release; + } + + revalidate() { + getValue(this.cache); + } +} /** @module @ember/debug @@ -28,7 +142,6 @@ import { Namespace, Object as EmberObject, A as emberA } from '@ember/-internals * `getRecordKeywords` * `getRecordFilterValues` * `getRecordColor` - * `observeRecord` The adapter will need to be registered in the application's container as `dataAdapter:main`. @@ -53,6 +166,9 @@ export default EmberObject.extend({ init() { this._super(...arguments); this.releaseMethods = emberA(); + this.recordsWatchers = new Map(); + this.typeWatchers = new Map(); + this.flushWatchers = null; }, /** @@ -92,6 +208,31 @@ export default EmberObject.extend({ */ acceptsModelName: true, + /** + Map from records arrays to RecordsWatcher instances + + @private + @property recordsWatchers + @since 3.26.0 + */ + + /** + Map from records arrays to TypeWatcher instances + + @private + @property typeWatchers + @since 3.26.0 + */ + + /** + Callback that is currently scheduled on backburner end to flush and check + all active watchers. + + @private + @property flushWatchers + @since 3.26.0 + */ + /** Stores all methods that clear observers. These methods will be called on destruction. @@ -100,7 +241,6 @@ export default EmberObject.extend({ @property releaseMethods @since 1.3.0 */ - releaseMethods: emberA(), /** Specifies how records can be filtered. @@ -179,58 +319,53 @@ export default EmberObject.extend({ Takes an array of objects containing wrapped records. @param {Function} recordsRemoved Callback to call when a record has removed. - Takes the following parameters: - index: The array index where the records were removed. - count: The number of records removed. + Takes an array of objects containing wrapped records. @return {Function} Method to call to remove all observers. */ watchRecords(modelName, recordsAdded, recordsUpdated, recordsRemoved) { - let releaseMethods = emberA(); let klass = this._nameToClass(modelName); let records = this.getRecords(klass, modelName); - let release; + let { recordsWatchers } = this; + + let recordsWatcher = recordsWatchers.get(records); + + if (!recordsWatcher) { + recordsWatcher = new RecordsWatcher( + records, + recordsAdded, + recordsUpdated, + recordsRemoved, + (record) => this.wrapRecord(record), + () => { + recordsWatchers.delete(records); + this.updateFlushWatchers(); + } + ); - function recordUpdated(updatedRecord) { - recordsUpdated([updatedRecord]); + recordsWatchers.set(records, recordsWatcher); + this.updateFlushWatchers(); + + recordsWatcher.revalidate(); } - let recordsToSend = records.map((record) => { - releaseMethods.push(this.observeRecord(record, recordUpdated)); - return this.wrapRecord(record); - }); + return recordsWatcher.release; + }, - let contentDidChange = (array, idx, removedCount, addedCount) => { - for (let i = idx; i < idx + addedCount; i++) { - let record = objectAt(array, i); - let wrapped = this.wrapRecord(record); - releaseMethods.push(this.observeRecord(record, recordUpdated)); - recordsAdded([wrapped]); - } + updateFlushWatchers() { + if (this.flushWatchers === null) { + if (this.typeWatchers.size > 0 || this.recordsWatchers.size > 0) { + this.flushWatchers = () => { + this.typeWatchers.forEach((watcher) => watcher.revalidate()); + this.recordsWatchers.forEach((watcher) => watcher.revalidate()); + }; - if (removedCount) { - recordsRemoved(idx, removedCount); + backburner.on('end', this.flushWatchers); } - }; - - let observer = { - didChange: contentDidChange, - willChange() { - return this; - }, - }; - addArrayObserver(records, this, observer); - - release = () => { - releaseMethods.forEach((fn) => fn()); - removeArrayObserver(records, this, observer); - this.releaseMethods.removeObject(release); - }; - - recordsAdded(recordsToSend); - - this.releaseMethods.pushObject(release); - return release; + } else if (this.typeWatchers.size === 0 && this.recordsWatchers.size === 0) { + backburner.off('end', this.flushWatchers); + this.flushWatchers = null; + } }, /** @@ -240,6 +375,10 @@ export default EmberObject.extend({ */ willDestroy() { this._super(...arguments); + + this.typeWatchers.forEach((watcher) => watcher.release()); + this.recordsWatchers.forEach((watcher) => watcher.release()); + this.releaseMethods.forEach((fn) => fn()); }, @@ -284,28 +423,27 @@ export default EmberObject.extend({ let klass = this._nameToClass(modelName); let records = this.getRecords(klass, modelName); - function onChange() { + let onChange = () => { typesUpdated([this.wrapModelType(klass, modelName)]); - } - - let observer = { - didChange(array, idx, removedCount, addedCount) { - // Only re-fetch records if the record count changed - // (which is all we care about as far as model types are concerned). - if (removedCount > 0 || addedCount > 0) { - scheduleOnce('actions', this, onChange); - } - }, - willChange() { - return this; - }, }; - addArrayObserver(records, this, observer); + let { typeWatchers } = this; - let release = () => removeArrayObserver(records, this, observer); + let typeWatcher = typeWatchers.get(records); - return release; + if (!typeWatcher) { + typeWatcher = new TypeWatcher(records, onChange, () => { + typeWatchers.delete(records); + this.updateFlushWatchers(); + }); + + typeWatchers.set(records, typeWatcher); + this.updateFlushWatchers(); + + typeWatcher.revalidate(); + } + + return typeWatcher.release; }, /** @@ -478,16 +616,4 @@ export default EmberObject.extend({ getRecordColor() { return null; }, - - /** - Observes all relevant properties and re-sends the wrapped record - when a change occurs. - - @public - @method observerRecord - @return {Function} The function to call to remove all observers. - */ - observeRecord() { - return function () {}; - }, }); diff --git a/packages/@ember/-internals/extension-support/tests/data_adapter_test.js b/packages/@ember/-internals/extension-support/tests/data_adapter_test.js index a6ae1f0b28b..9dabc566616 100644 --- a/packages/@ember/-internals/extension-support/tests/data_adapter_test.js +++ b/packages/@ember/-internals/extension-support/tests/data_adapter_test.js @@ -1,5 +1,5 @@ import { run } from '@ember/runloop'; -import { get, set, addObserver, removeObserver } from '@ember/-internals/metal'; +import { get, set } from '@ember/-internals/metal'; import { Object as EmberObject, A as emberA } from '@ember/-internals/runtime'; import EmberDataAdapter from '../lib/data_adapter'; import { moduleFor, ApplicationTestCase, runLoopSettled } from 'internal-test-helpers'; @@ -159,36 +159,6 @@ moduleFor( }); } - ['@test Model Types Updated but Unchanged Do not Trigger Callbacks'](assert) { - assert.expect(0); - let records = emberA([1, 2, 3]); - this.add( - 'data-adapter:main', - DataAdapter.extend({ - getRecords() { - return records; - }, - }) - ); - this.add('model:post', PostClass); - - return this.visit('/').then(() => { - adapter = this.applicationInstance.lookup('data-adapter:main'); - - function modelTypesAdded() { - run(() => { - records.arrayContentDidChange(0, 0, 0); - }); - } - - function modelTypesUpdated() { - assert.ok(false, "modelTypesUpdated should not be triggered if the array didn't change"); - } - - adapter.watchModelTypes(modelTypesAdded, modelTypesUpdated); - }); - } - ['@test Records Added'](assert) { let countAdded = 1; let post = PostClass.create(); @@ -250,16 +220,6 @@ moduleFor( getRecords() { return recordList; }, - observeRecord(record, recordUpdated) { - let self = this; - function callback() { - recordUpdated(self.wrapRecord(record)); - } - addObserver(record, 'title', callback); - return function () { - removeObserver(record, 'title', callback); - }; - }, getRecordColumnValues(record) { return { title: get(record, 'title') }; }, @@ -273,8 +233,8 @@ moduleFor( .then(() => { adapter = this.applicationInstance.lookup('data-adapter:main'); - function recordsAdded() { - set(post, 'title', 'Post Modified'); + function recordsAdded(records) { + assert.equal(records[0].columnValues.title, 'Post', 'Post added correctly'); } function recordsUpdated(records) { @@ -286,6 +246,11 @@ moduleFor( return runLoopSettled(); }) + .then(() => { + set(post, 'title', 'Post Modified'); + + return runLoopSettled(); + }) .then(() => { release(); set(post, 'title', 'New Title'); diff --git a/tests/docs/expected.js b/tests/docs/expected.js index 7656fc6bd92..71a48b47911 100644 --- a/tests/docs/expected.js +++ b/tests/docs/expected.js @@ -115,9 +115,9 @@ module.exports = { 'cacheFor', 'camelize', 'canCatalogEntriesByType', - 'canInvoke', 'cancel', 'cancelRouterSetup', + 'canInvoke', 'capabilities', 'capitalize', 'captureRenderTree', @@ -127,9 +127,9 @@ module.exports = { 'checkWaiters', 'child', 'childViews', + 'classify', 'classNameBindings', 'classNames', - 'classify', 'clear', 'click', 'cloneParentDependencies', @@ -151,6 +151,7 @@ module.exports = { 'controllerName', 'copy', 'create', + 'createCache', 'current-when', 'currentPath', 'currentRoute', @@ -195,8 +196,8 @@ module.exports = { 'disconnectOutlet', 'document', 'domReady', - 'each', 'each-in', + 'each', 'eachComputedProperty', 'element', 'elementId', @@ -228,6 +229,7 @@ module.exports = { 'findModel', 'findWithAssert', 'firstObject', + 'flushWatchers', 'fn', 'focusIn', 'focusOut', @@ -255,6 +257,7 @@ module.exports = { 'getRecords', 'getRootViews', 'getURL', + 'getValue', 'getViewBoundingClientRect', 'getViewBounds', 'getViewClientRects', @@ -272,12 +275,12 @@ module.exports = { 'hasArrayObservers', 'hasBlock', 'hasBlockParams', + 'hash', + 'hashSettled', 'hasListeners', 'hasObserverFor', 'hasRegistration', 'hasRoute', - 'hash', - 'hashSettled', 'helper', 'helperContainer', 'history', @@ -290,11 +293,11 @@ module.exports = { 'indexOf', 'info', 'init', - 'initState', 'initializer', + 'initState', 'inject', - 'injectTestHelpers', 'injection', + 'injectTestHelpers', 'input', 'insertAt', 'insertNewline', @@ -313,6 +316,7 @@ module.exports = { 'isBrowser', 'isClassicDecorator', 'isComputed', + 'isConst', 'isDestroyed', 'isDestroying', 'isEmpty', @@ -329,8 +333,8 @@ module.exports = { 'isRejected', 'isSettled', 'isVisible', - 'jQuery', 'join', + 'jQuery', 'keyDown', 'keyEvent', 'keyPress', @@ -391,16 +395,15 @@ module.exports = { 'objectsAt', 'observeModelType', 'observer', - 'observerRecord', 'observes', 'off', 'on', - 'onInjectHelpers', - 'onLoad', - 'onUpdateURL', 'once', 'one', 'oneWay', + 'onInjectHelpers', + 'onLoad', + 'onUpdateURL', 'options', 'optionsForType', 'or', @@ -431,14 +434,15 @@ module.exports = { 'queues', 'race', 'readDOMAttr', - 'readOnly', 'readonly', + 'readOnly', 'reads', 'ready', 'reason', 'recognize', 'recognizeAndLoad', 'recompute', + 'recordsWatchers', 'redirect', 'reduce', 'refresh', @@ -446,15 +450,15 @@ module.exports = { 'registerAsyncHelper', 'registerDeprecationHandler', 'registerDestructor', + 'registeredActions', + 'registeredOption', + 'registeredOptions', + 'registeredOptionsForType', 'registerHelper', 'registerOptions', 'registerOptionsForType', 'registerWaiter', 'registerWarnHandler', - 'registeredActions', - 'registeredOption', - 'registeredOptions', - 'registeredOptionsForType', 'registrations', 'registry', 'reject', @@ -486,12 +490,12 @@ module.exports = { 'resolveHelper', 'resolveModel', 'resolveOther', + 'resolver', 'resolveRegistration', + 'resolverFor', 'resolveRoute', 'resolveTemplate', 'resolveView', - 'resolver', - 'resolverFor', 'resumeTest', 'rethrow', 'retry', @@ -524,12 +528,12 @@ module.exports = { 'setObjects', 'setOwner', 'setProperties', - 'setURL', 'setup', 'setupController', 'setupForTesting', 'setupHandler', 'setupRegistry', + 'setURL', 'shiftObject', 'shouldRender', 'size', @@ -555,8 +559,8 @@ module.exports = { 'title', 'to', 'toArray', - 'toString', 'toggleProperty', + 'toString', 'tracked', 'transitionTo', 'transitionToRoute', @@ -569,6 +573,7 @@ module.exports = { 'type', 'typeInjection', 'typeOf', + 'typeWatchers', 'unbound', 'underscore', 'union', @@ -584,8 +589,8 @@ module.exports = { 'unsubscribe', 'url', 'urlFor', - 'useRouterNaming', 'userAgent', + 'useRouterNaming', 'validationCache', 'value', 'visit', @@ -609,8 +614,5 @@ module.exports = { 'wrapModelType', 'wrapRecord', 'yield', - 'createCache', - 'getValue', - 'isConst', ], };