From b0d0aa49b3b6c01f4385d1b5c7f72ee1e6d6f013 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 22 Apr 2021 01:35:41 -0700 Subject: [PATCH 1/3] mostly working --- .../adapter/build-url-mixin-test.js | 156 ++++++++-- packages/-ember-data/tests/unit/model-test.js | 34 ++- packages/model/addon/-private/attr.js | 61 ++-- packages/model/addon/-private/belongs-to.js | 89 +----- packages/model/addon/-private/has-many.js | 72 +---- packages/model/addon/-private/model.js | 269 +++++++++++++----- packages/model/addon/-private/util.ts | 141 ++++++++- .../-private/system/model/internal-model.ts | 34 ++- .../store/types/@glimmer/tracking/index.d.ts | 3 + packages/store/types/ember/index.d.ts | 7 + 10 files changed, 545 insertions(+), 321 deletions(-) create mode 100644 packages/store/types/@glimmer/tracking/index.d.ts diff --git a/packages/-ember-data/tests/integration/adapter/build-url-mixin-test.js b/packages/-ember-data/tests/integration/adapter/build-url-mixin-test.js index f3a294f0e5c..a74f17c44e1 100644 --- a/packages/-ember-data/tests/integration/adapter/build-url-mixin-test.js +++ b/packages/-ember-data/tests/integration/adapter/build-url-mixin-test.js @@ -14,7 +14,7 @@ import deepCopy from '@ember-data/unpublished-test-infra/test-support/deep-copy' module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', function(hooks) { setupTest(hooks); - let store, adapter, Post, Comment, passedUrl; + let store, adapter, passedUrl; function ajaxResponse(value) { adapter.ajax = function(url, verb, hash) { @@ -24,32 +24,31 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }; } + class Post extends Model { + @attr name; + } + class Comment extends Model { + @attr name; + } + hooks.beforeEach(function() { let { owner } = this; - const PostModel = Model.extend({ - name: attr('string'), - }); - const CommentModel = Model.extend({ - name: attr('string'), - }); const SuperUser = Model.extend({}); owner.register('adapter:application', RESTAdapter.extend()); owner.register('serializer:application', RESTSerializer.extend()); - owner.register('model:comment', CommentModel); - owner.register('model:post', PostModel); owner.register('model:super-user', SuperUser); store = owner.lookup('service:store'); adapter = store.adapterFor('application'); - Post = store.modelFor('post'); - Comment = store.modelFor('comment'); - passedUrl = null; }); test('buildURL - with host and namespace', async function(assert) { + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); + adapter.setProperties({ host: 'http://example.com', namespace: 'api/v1', @@ -68,8 +67,16 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f namespace: 'api/v1', }); - Post.reopen({ comments: hasMany('comment', { async: true }) }); - Comment.reopen({ post: belongsTo('post', { async: false }) }); + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); ajaxResponse({ posts: [{ id: 1, links: { comments: 'comments' } }] }); @@ -86,8 +93,16 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f namespace: 'api/v1', }); - Post.reopen({ comments: hasMany('comment', { async: true }) }); - Comment.reopen({ post: belongsTo('post', { async: false }) }); + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); ajaxResponse({ posts: [{ id: 1, links: { comments: '/api/v1/posts/1/comments' } }] }); @@ -103,8 +118,17 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f host: '//example.com', namespace: 'api/v1', }); - Post.reopen({ comments: hasMany('comment', { async: true }) }); - Comment.reopen({ post: belongsTo('post', { async: false }) }); + + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); ajaxResponse({ posts: [{ id: 1, links: { comments: '/api/v1/posts/1/comments' } }] }); @@ -120,8 +144,17 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f host: '/', namespace: 'api/v1', }); - Post.reopen({ comments: hasMany('comment', { async: true }) }); - Comment.reopen({ post: belongsTo('post', { async: false }) }); + + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); ajaxResponse({ posts: [{ id: 1, links: { comments: '/api/v1/posts/1/comments' } }] }); @@ -137,8 +170,16 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f host: 'http://example.com', namespace: 'api/v1', }); - Post.reopen({ comments: hasMany('comment', { async: true }) }); - Comment.reopen({ post: belongsTo('post', { async: false }) }); + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); ajaxResponse({ posts: [ @@ -171,7 +212,15 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }); test('buildURL - buildURL takes a record from find', async function(assert) { - Comment.reopen({ post: belongsTo('post', { async: false }) }); + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + class Post extends Model { + @attr name; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); adapter.buildURL = function(type, id, snapshot) { return '/posts/' + snapshot.belongsTo('post', { id: true }) + '/comments/' + snapshot.id; @@ -186,14 +235,22 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }, }); - await store.findRecord('comment', 1, { preload: { post } }); + await store.findRecord('comment', '1', { preload: { post } }); assert.equal(passedUrl, '/posts/2/comments/1'); }); test('buildURL - buildURL takes the records from findMany', async function(assert) { - Comment.reopen({ post: belongsTo('post', { async: false }) }); - Post.reopen({ comments: hasMany('comment', { async: true }) }); + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); adapter.buildURL = function(type, ids, snapshots) { if (Array.isArray(snapshots)) { @@ -225,7 +282,16 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }); test('buildURL - buildURL takes a record from create', async function(assert) { - Comment.reopen({ post: belongsTo('post', { async: false }) }); + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); adapter.buildURL = function(type, id, snapshot) { return '/posts/' + snapshot.belongsTo('post', { id: true }) + '/comments/'; }; @@ -245,7 +311,16 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }); test('buildURL - buildURL takes a record from create to query a resolved async belongsTo relationship', async function(assert) { - Comment.reopen({ post: belongsTo('post', { async: true }) }); + class Comment extends Model { + @attr name; + @belongsTo('post', { async: true }) post; + } + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); adapter.buildURL = function(type, id, snapshot) { return '/posts/' + snapshot.belongsTo('post', { id: true }) + '/comments/'; }; @@ -271,7 +346,16 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }); test('buildURL - buildURL takes a record from update', async function(assert) { - Comment.reopen({ post: belongsTo('post', { async: false }) }); + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + class Post extends Model { + @attr name; + @hasMany('comment', { async: true }) comments; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); adapter.buildURL = function(type, id, snapshot) { return '/posts/' + snapshot.belongsTo('post', { id: true }) + '/comments/' + snapshot.id; }; @@ -297,8 +381,17 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }); test('buildURL - buildURL takes a record from delete', async function(assert) { - Comment.reopen({ post: belongsTo('post', { async: false }) }); - Post.reopen({ comments: hasMany('comment', { async: false }) }); + class Post extends Model { + @attr name; + @hasMany('comment', { async: false }) comments; + } + class Comment extends Model { + @attr name; + @belongsTo('post', { async: false }) post; + } + this.owner.register('model:comment', Comment); + this.owner.register('model:post', Post); + adapter.buildURL = function(type, id, snapshot) { return 'posts/' + snapshot.belongsTo('post', { id: true }) + '/comments/' + snapshot.id; }; @@ -326,6 +419,7 @@ module('integration/adapter/build-url-mixin - BuildURLMixin with RESTAdapter', f }); test('buildURL - with absolute namespace', async function(assert) { + this.owner.register('model:post', Post); adapter.setProperties({ namespace: '/api/v1', }); diff --git a/packages/-ember-data/tests/unit/model-test.js b/packages/-ember-data/tests/unit/model-test.js index ca0d038dc8e..64d95140e99 100644 --- a/packages/-ember-data/tests/unit/model-test.js +++ b/packages/-ember-data/tests/unit/model-test.js @@ -742,21 +742,25 @@ module('unit/model - Model', function(hooks) { }, /Cannot set property isLoaded of \[object Object\] which has only a getter/); }); - class NativePostWithInternalModel extends Model { - @attr('string') - _internalModel; - @attr('string') - name; - } - class NativePostWithCurrentState extends Model { - @attr('string') - currentState; - @attr('string') - name; - } + const makeNativePostWithInternalModel = () => { + return class NativePostWithInternalModel extends Model { + @attr('string') + _internalModel; + @attr('string') + name; + }; + }; + const makeNativePostWithCurrentState = () => { + return class NativePostWithCurrentState extends Model { + @attr('string') + currentState; + @attr('string') + name; + }; + }; const PROP_MAP = { - _internalModel: NativePostWithInternalModel, - currentState: NativePostWithCurrentState, + _internalModel: makeNativePostWithInternalModel, + currentState: makeNativePostWithCurrentState, }; function testReservedProperty(prop) { @@ -775,7 +779,7 @@ module('unit/model - Model', function(hooks) { assert.throws( () => { - store.createRecord('native-post', { name: 'TomHuda' }); + NativePost(); }, function(e) { return e.message.indexOf(msg) === 0; diff --git a/packages/model/addon/-private/attr.js b/packages/model/addon/-private/attr.js index 1bba908d9ab..726b9758f31 100644 --- a/packages/model/addon/-private/attr.js +++ b/packages/model/addon/-private/attr.js @@ -1,11 +1,9 @@ import { assert } from '@ember/debug'; -import { computed } from '@ember/object'; -import { DEBUG } from '@glimmer/env'; import { RECORD_DATA_ERRORS } from '@ember-data/canary-features'; import { recordDataFor } from '@ember-data/store/-private'; -import { computedMacroWithOptionalParams } from './util'; +import { makeDecorator } from './util'; /** @module @ember-data/model @@ -13,7 +11,7 @@ import { computedMacroWithOptionalParams } from './util'; function getDefaultValue(record, options, key) { if (typeof options.defaultValue === 'function') { - return options.defaultValue.apply(null, arguments); + return options.defaultValue.call(null, record, options, key); } else { let defaultValue = options.defaultValue; assert( @@ -112,45 +110,20 @@ function getDefaultValue(record, options, key) { @param {Object} options a hash of options @return {Attribute} */ -function attr(type, options) { - if (typeof type === 'object') { - options = type; - type = undefined; - } else { - options = options || {}; - } - - let meta = { - type: type, - isAttribute: true, - kind: 'attribute', - options: options, - }; - - return computed({ - get(key) { - if (DEBUG) { - if (['_internalModel', 'currentState'].indexOf(key) !== -1) { - throw new Error( - `'${key}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your attr on ${this.constructor.toString()}` - ); - } - } +export default makeDecorator('attribute', { + getter(key, meta) { + return function() { let recordData = recordDataFor(this); if (recordData.hasAttr(key)) { - return recordData.getAttr(key); + let v = recordData.getAttr(key); + return v; } else { - return getDefaultValue(this, options, key); - } - }, - set(key, value) { - if (DEBUG) { - if (['_internalModel', 'currentState'].indexOf(key) !== -1) { - throw new Error( - `'${key}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your attr on ${this.constructor.toString()}` - ); - } + return getDefaultValue(this, meta.options, key); } + }; + }, + setter(key) { + return function(value) { if (RECORD_DATA_ERRORS) { let oldValue = this._internalModel._recordData.getAttr(key); if (oldValue !== value) { @@ -161,9 +134,7 @@ function attr(type, options) { this._markInvalidRequestAsClean(); } } - return this._internalModel.setDirtyAttribute(key, value); - }, - }).meta(meta); -} - -export default computedMacroWithOptionalParams(attr); + this._internalModel.setDirtyAttribute(key, value); + }; + }, +}); diff --git a/packages/model/addon/-private/belongs-to.js b/packages/model/addon/-private/belongs-to.js index 66b6f37c6f7..9a2a2af6010 100644 --- a/packages/model/addon/-private/belongs-to.js +++ b/packages/model/addon/-private/belongs-to.js @@ -1,9 +1,4 @@ -import { assert, inspect, warn } from '@ember/debug'; -import { computed } from '@ember/object'; -import { DEBUG } from '@glimmer/env'; - -import { computedMacroWithOptionalParams } from './util'; - +import { makeDecorator } from './util'; /** @module @ember-data/model */ @@ -109,78 +104,16 @@ import { computedMacroWithOptionalParams } from './util'; @param {Object} options (optional) a hash of options @return {Ember.computed} relationship */ -function belongsTo(modelName, options) { - let opts, userEnteredModelName; - if (typeof modelName === 'object') { - opts = modelName; - userEnteredModelName = undefined; - } else { - opts = options; - userEnteredModelName = modelName; - } - - assert( - 'The first argument to belongsTo must be a string representing a model type key, not an instance of ' + - inspect(userEnteredModelName) + - ". E.g., to define a relation to the Person model, use belongsTo('person')", - typeof userEnteredModelName === 'string' || typeof userEnteredModelName === 'undefined' - ); - - opts = opts || {}; - - let meta = { - type: userEnteredModelName, - isRelationship: true, - options: opts, - kind: 'belongsTo', - name: 'Belongs To', - key: null, - }; - - return computed({ - get(key) { - if (DEBUG) { - if (['_internalModel', 'recordData', 'currentState'].indexOf(key) !== -1) { - throw new Error( - `'${key}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your belongsTo on ${this.constructor.toString()}` - ); - } - if (Object.prototype.hasOwnProperty.call(opts, 'serialize')) { - warn( - `You provided a serialize option on the "${key}" property in the "${this._internalModel.modelName}" class, this belongs in the serializer. See Serializer and it's implementations https://api.emberjs.com/ember-data/release/classes/Serializer`, - false, - { - id: 'ds.model.serialize-option-in-belongs-to', - } - ); - } - - if (Object.prototype.hasOwnProperty.call(opts, 'embedded')) { - warn( - `You provided an embedded option on the "${key}" property in the "${this._internalModel.modelName}" class, this belongs in the serializer. See EmbeddedRecordsMixin https://api.emberjs.com/ember-data/release/classes/EmbeddedRecordsMixin`, - false, - { - id: 'ds.model.embedded-option-in-belongs-to', - } - ); - } - } +export default makeDecorator('belongsTo', { + getter(key) { + return function() { return this._internalModel.getBelongsTo(key); - }, - set(key, value) { - if (DEBUG) { - if (['_internalModel', 'recordData', 'currentState'].indexOf(key) !== -1) { - throw new Error( - `'${key}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your belongsTo on ${this.constructor.toString()}` - ); - } - } + }; + }, + setter(key) { + return function(value) { this._internalModel.setDirtyBelongsTo(key, value); - - return this._internalModel.getBelongsTo(key); - }, - }).meta(meta); -} - -export default computedMacroWithOptionalParams(belongsTo); + }; + }, +}); diff --git a/packages/model/addon/-private/has-many.js b/packages/model/addon/-private/has-many.js index 9c61011d1ce..81cbbf3c740 100644 --- a/packages/model/addon/-private/has-many.js +++ b/packages/model/addon/-private/has-many.js @@ -1,11 +1,7 @@ +import { makeDecorator } from './util'; /** @module @ember-data/model */ -import { assert, inspect } from '@ember/debug'; -import { computed } from '@ember/object'; -import { DEBUG } from '@glimmer/env'; - -import { computedMacroWithOptionalParams } from './util'; /** `hasMany` is used to define One-To-Many and Many-To-Many @@ -149,59 +145,15 @@ import { computedMacroWithOptionalParams } from './util'; @param {Object} options (optional) a hash of options @return {Ember.computed} relationship */ -function hasMany(type, options) { - if (typeof type === 'object') { - options = type; - type = undefined; - } - - assert( - `The first argument to hasMany must be a string representing a model type key, not an instance of ${inspect( - type - )}. E.g., to define a relation to the Comment model, use hasMany('comment')`, - typeof type === 'string' || typeof type === 'undefined' - ); - - options = options || {}; - - // Metadata about relationships is stored on the meta of - // the relationship. This is used for introspection and - // serialization. Note that `key` is populated lazily - // the first time the CP is called. - let meta = { - type, - options, - isRelationship: true, - kind: 'hasMany', - name: 'Has Many', - key: null, - }; - - return computed({ - get(key) { - if (DEBUG) { - if (['_internalModel', 'recordData', 'currentState'].indexOf(key) !== -1) { - throw new Error( - `'${key}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your hasMany on ${this.constructor.toString()}` - ); - } - } +export default makeDecorator('hasMany', { + getter(key) { + return function() { return this._internalModel.getHasMany(key); - }, - set(key, records) { - if (DEBUG) { - if (['_internalModel', 'recordData', 'currentState'].indexOf(key) !== -1) { - throw new Error( - `'${key}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your hasMany on ${this.constructor.toString()}` - ); - } - } - let internalModel = this._internalModel; - internalModel.setDirtyHasMany(key, records); - - return internalModel.getHasMany(key); - }, - }).meta(meta); -} - -export default computedMacroWithOptionalParams(hasMany); + }; + }, + setter(key) { + return function(records) { + this._internalModel.setDirtyHasMany(key, records); + }; + }, +}); diff --git a/packages/model/addon/-private/model.js b/packages/model/addon/-private/model.js index 074e164ef3c..e572877e092 100644 --- a/packages/model/addon/-private/model.js +++ b/packages/model/addon/-private/model.js @@ -1,9 +1,10 @@ import { assert, deprecate, warn } from '@ember/debug'; import EmberError from '@ember/error'; -import EmberObject, { computed, get } from '@ember/object'; +import EmberObject, { computed } from '@ember/object'; import { dependentKeyCompat } from '@ember/object/compat'; import { isNone } from '@ember/utils'; import { DEBUG } from '@glimmer/env'; +import { tracked } from '@glimmer/tracking'; import Ember from 'ember'; import { RECORD_DATA_ERRORS, RECORD_DATA_STATE, REQUEST_SERVICE } from '@ember-data/canary-features'; @@ -26,17 +27,78 @@ import { import Errors from './errors'; import { relationshipFromMeta } from './system/relationships/relationship-meta'; +const DEBUG__STR = Date.now(); const RecordMeta = new WeakMap(); -function getRecordMeta(record) { - let meta = RecordMeta.get(record); - if (meta === undefined) { - meta = Object.create(null); - RecordMeta.set(record, meta); +const SchemaCache = new WeakMap(); + +function cacheFor(map, key, debugProp) { + let v = map.get(key); + + if (!v) { + v = Object.create(null); + map.set(key, v); + if (DEBUG) { - record.____recordMetaCache = meta; + key[`${debugProp}--${DEBUG__STR}`] = v; } } - return meta; + + return v; +} + +export function getRecordMeta(record) { + return cacheFor(RecordMeta, record, 'recordMeta'); +} + +export function schemaCacheFor(klass) { + return cacheFor(SchemaCache, klass, 'schemaCache'); +} + +class Tag { + constructor() { + this.rev = 0; + this.isDirty = true; + this.value = undefined; + } + @tracked dirty = ''; + + notify() { + this.isDirty = true; + this.dirty = ''; + this.rev++; + } + consume(v) { + this.dirty; + this.isDirty = false; + this.value = v; + } +} + +export function getTag(record, key: string) { + const rm = getRecordMeta(record); + rm.tags = rm.tags || {}; + return (rm.tags[key] = rm.tags[key] || new Tag()); +} + +export function tagged(target, key, desc) { + const getter = desc.get; + const setter = desc.set; + desc.get = function() { + let tag = getTag(this, key); + if (tag.isDirty && !this.isDestroyed && !this.isDestroying) { + tag.consume(getter.call(this)); + } + + return tag.value; + }; + if (setter) { + desc.set = function(value) { + let tag = getTag(this, key); + setter.call(this, value); + tag.notify(); + }; + } + return desc; } function getWithDefault(meta, prop, value) { @@ -91,20 +153,22 @@ function findPossibleInverses(type, inverseType, name, relationshipsSoFar) { * an expensive getter on first-access and therafter * never recompute it. */ +const STATIC_KEY_RECALC = `static${Date.now()}`; function computeOnce(target, key, desc) { const cache = new WeakMap(); let getter = desc.get; desc.get = function() { + let tag = getTag(this, STATIC_KEY_RECALC); let meta = cache.get(this); if (!meta) { - meta = { hasComputed: false, value: undefined }; + meta = { rev: null, value: undefined }; cache.set(this, meta); } - if (!meta.hasComputed) { + if (meta.rev !== tag.rev) { meta.value = getter.call(this); - meta.hasComputed = true; + meta.rev = tag.rev; } return meta.value; @@ -164,6 +228,11 @@ class Model extends EmberObject { } } + @tagged + get currentState() { + return this._internalModel.currentState; + } + /** If this property is `true` the record is in the `empty` state. Empty is the first state all records enter after they have @@ -179,7 +248,7 @@ class Model extends EmberObject { */ @dependentKeyCompat get isEmpty() { - return this._internalModel.currentState.isEmpty; + return this.currentState.isEmpty; } /** @@ -194,7 +263,7 @@ class Model extends EmberObject { */ @dependentKeyCompat get isLoading() { - return this._internalModel.currentState.isLoading; + return this.currentState.isLoading; } /** If this property is `true` the record is in the `loaded` state. A @@ -219,7 +288,7 @@ class Model extends EmberObject { */ @dependentKeyCompat get isLoaded() { - return this._internalModel.currentState.isLoaded; + return this.currentState.isLoaded; } /** @@ -248,7 +317,7 @@ class Model extends EmberObject { */ @dependentKeyCompat get hasDirtyAttributes() { - return this._internalModel.currentState.isDirty; + return this.currentState.isDirty; } /** @@ -275,7 +344,7 @@ class Model extends EmberObject { */ @dependentKeyCompat get isSaving() { - return this._internalModel.currentState.isSaving; + return this.currentState.isSaving; } /** @@ -328,7 +397,18 @@ class Model extends EmberObject { } } - return this._internalModel.currentState.isDeleted; + return this.currentState.isDeleted; + } + + notifyPropertyChange(key, silent) { + let meta = getRecordMeta(this); + if (meta.tags && meta.tags[key]) { + if (!silent) { + meta.tags[key].notify(); + } + } else { + super.notifyPropertyChange(key); + } } /** @@ -365,7 +445,7 @@ class Model extends EmberObject { } } - return this._internalModel.currentState.isNew; + return this.currentState.isNew; } /** @@ -384,7 +464,7 @@ class Model extends EmberObject { return !(this.errors.length > 0); } - return this._internalModel.currentState.isValid; + return this.currentState.isValid; } _markInvalidRequestAsClean() { @@ -416,7 +496,7 @@ class Model extends EmberObject { */ @dependentKeyCompat get dirtyType() { - return this._internalModel.currentState.dirtyType; + return this.currentState.dirtyType; } /** @@ -868,6 +948,9 @@ class Model extends EmberObject { @private */ _notifyProperties(keys) { + deprecate(`notifyProperties`, false, { + id: '@ember-data/model:deprecate-model#_notifyProperties', + }); // changeProperties defers notifications until after the delegate // and protects with a try...finally block // previously used begin...endPropertyChanges but this is private API @@ -1196,6 +1279,9 @@ class Model extends EmberObject { } notifyBelongsToChange(key) { + deprecate(`notifyBelongsToChange`, false, { + id: '@ember-data/model:deprecate-model#notifyBelongsToChange', + }); this.notifyPropertyChange(key); } /** @@ -1263,6 +1349,9 @@ class Model extends EmberObject { } notifyHasManyAdded(key) { + deprecate(`notifyHasManyAdded`, false, { + id: '@ember-data/model:deprecate-model#notifyHasManyAdded', + }); //We need to notifyPropertyChange in the adding case because we need to make sure //we fetch the newly added record in case it is unloaded //TODO(Igor): Consider whether we could do this only if the record state is unloaded @@ -1410,6 +1499,11 @@ class Model extends EmberObject { } } + static metaForProperty(name) { + let rel = this.relationshipsByName.get(name); + return rel ? rel.meta : this.attributes.get(name); + } + //Calculate the inverse, ignoring the cache static _findInverseFor(name, store) { let inverseType = this.typeForRelationship(name, store); @@ -1652,27 +1746,22 @@ class Model extends EmberObject { @property relatedTypes @static - @type Ember.Array + @type Array @readOnly */ @computeOnce static get relatedTypes() { let types = []; - let rels = this.relationshipsObject; - let relationships = Object.keys(rels); - // create an array of the unique types involved // in relationships - for (let i = 0; i < relationships.length; i++) { - let name = relationships[i]; - let meta = rels[name]; + this.relationshipsByName.forEach((meta, name) => { let modelName = meta.type; if (types.indexOf(modelName) === -1) { types.push(modelName); } - } + }); return types; } @@ -1710,36 +1799,19 @@ class Model extends EmberObject { @property relationshipsByName @static - @type Map + @type @readOnly */ @computeOnce static get relationshipsByName() { - let map = new Map(); - let rels = this.relationshipsObject; - let relationships = Object.keys(rels); - - for (let i = 0; i < relationships.length; i++) { - let key = relationships[i]; - let value = rels[key]; - - map.set(value.key, value); - } - - return map; + return walkProtoForSchema(this).relationships; } @computeOnce static get relationshipsObject() { let relationships = Object.create(null); - let modelName = this.modelName; - this.eachComputedProperty((name, meta) => { - if (meta.isRelationship) { - meta.key = name; - meta.name = name; - meta.parentModelName = modelName; - relationships[name] = relationshipFromMeta(meta); - } + this.relationshipsByName.forEach((meta, name) => { + relationships[name] = meta; }); return relationships; } @@ -1787,17 +1859,7 @@ class Model extends EmberObject { */ @computeOnce static get fields() { - let map = new Map(); - - this.eachComputedProperty((name, meta) => { - if (meta.isRelationship) { - map.set(name, meta.kind); - } else if (meta.isAttribute) { - map.set(name, 'attribute'); - } - }); - - return map; + return walkProtoForSchema(this).fields; } /** @@ -1897,22 +1959,7 @@ class Model extends EmberObject { */ @computeOnce static get attributes() { - let map = new Map(); - - this.eachComputedProperty((name, meta) => { - if (meta.isAttribute) { - assert( - "You may not set `id` as an attribute on your model. Please remove any lines that look like: `id: attr('')` from " + - this.toString(), - name !== 'id' - ); - - meta.name = name; - map.set(name, meta); - } - }); - - return map; + return walkProtoForSchema(this).attributes; } /** @@ -2065,6 +2112,11 @@ class Model extends EmberObject { }); } + static reopen(...args) { + getTag(this, STATIC_KEY_RECALC).notify(); + super.reopen(...args); + } + /** Returns the name of the model class. @@ -2072,14 +2124,17 @@ class Model extends EmberObject { @static */ static toString() { - return `model:${get(this, 'modelName')}`; + let name = this.modelName; + if (!name && this.name !== 'Function') { + name = this.name; + } + return `model:${name}`; } } // this is required to prevent `init` from passing // the values initialized during create to `setUnknownProperty` Model.prototype._internalModel = null; -Model.prototype.currentState = null; Model.prototype.store = null; if (HAS_DEBUG_PACKAGE) { @@ -2283,7 +2338,10 @@ if (DEBUG) { ); } - if (!isDefaultEmptyDescriptor(this, 'currentState') || this.currentState !== this._internalModel.currentState) { + if ( + lookupDescriptor(Model.prototype, 'currentState').get !== lookupDescriptor(this, 'currentState').get || + this._internalModel.currentState !== this.currentState + ) { throw new Error( `'currentState' is a reserved property name on instances of classes extending Model. Please choose a different property name for ${this.constructor.toString()}` ); @@ -2326,4 +2384,57 @@ if (DEBUG) { }); } +function walkProtoForSchema(klass) { + const tag = getTag(klass, STATIC_KEY_RECALC); + const klassCache = schemaCacheFor(klass); + + if (klassCache._protoCache && !tag.isDirty) { + return klassCache._protoCache; + } + const attributes = new Map(); + const relationships = new Map(); + const fields = new Map(); + + klassCache._protoCache = { + attributes, + relationships, + fields, + }; + const modelName = klass.modelName; + + if (!modelName || tag.isDirty) { + klass.proto(); // allow us to be called without store.modelFor + tag.isDirty = false; + } + + let cache = klassCache; + let inheritedKlass = klass; + do { + if (inheritedKlass === Model) { + break; + } + cache = schemaCacheFor(inheritedKlass); + if (cache.attributes) { + cache.attributes.forEach((v, k) => { + if (!attributes.has(k)) { + attributes.set(k, v); + fields.set(k, 'attribute'); + } + }); + } + if (cache.relationships) { + cache.relationships.forEach((v, k) => { + if (!relationships.has(k)) { + v.parentModelName = modelName; + let meta = relationshipFromMeta(v); + relationships.set(k, meta); + fields.set(k, v.kind); + } + }); + } + } while ((inheritedKlass = inheritedKlass.__proto__)); + + return klassCache._protoCache; +} + export default Model; diff --git a/packages/model/addon/-private/util.ts b/packages/model/addon/-private/util.ts index 314296642cc..3267d029c89 100644 --- a/packages/model/addon/-private/util.ts +++ b/packages/model/addon/-private/util.ts @@ -1,8 +1,18 @@ +import { assert, deprecate, inspect, warn } from '@ember/debug'; +import { DEBUG } from '@glimmer/env'; +import Ember from 'ember'; + import { gte } from 'ember-compatibility-helpers'; -export type DecoratorPropertyDescriptor = (PropertyDescriptor & { initializer?: any }) | undefined; +import { schemaCacheFor, tagged } from './model'; + +type Dict = import('@ember-data/store/-private/ts-interfaces/utils').Dict; + +const { _setClassicDecorator } = Ember; -export function isElementDescriptor(args: any[]): args is [object, string, DecoratorPropertyDescriptor] { +type DecoratorPropertyDescriptor = (PropertyDescriptor & { initializer?: any }) | undefined; + +function isElementDescriptor(args: any[]): args is [object, string, DecoratorPropertyDescriptor] { let [maybeTarget, maybeKey, maybeDesc] = args; return ( @@ -22,10 +32,135 @@ export function isElementDescriptor(args: any[]): args is [object, string, Decor ); } -export function computedMacroWithOptionalParams(fn) { +function computedMacroWithOptionalParams(fn) { if (gte('3.10.0')) { return (...maybeDesc: any[]) => (isElementDescriptor(maybeDesc) ? fn()(...maybeDesc) : fn(...maybeDesc)); } else { return fn; } } + +interface PropertyMeta { + type?: string; + options: Dict; + kind: string; + name: string; + /** + * @deprecated + */ + key: string; + isRelationship?: true; + isAttribute?: true; +} + +const assertProps = + DEBUG && + function assertProps(target, meta) { + const { name, kind, options } = meta; + if (['_internalModel', 'recordData', 'currentState'].indexOf(name) !== -1) { + throw new Error( + `'${name}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your belongsTo on ${target.toString()}` + ); + } + if (kind !== 'attribute') { + if (Object.prototype.hasOwnProperty.call(options, 'serialize')) { + warn( + `You provided a serialize option on the "${name}" property in the "${target.toString()}" class, this belongs in the serializer. See Serializer and it's implementations https://api.emberjs.com/ember-data/release/classes/Serializer`, + false, + { + id: 'ds.model.serialize-option-in-belongs-to', + } + ); + } + if (Object.prototype.hasOwnProperty.call(options, 'embedded')) { + warn( + `You provided an embedded option on the "${name}" property in the "${target.toString()}" class, this belongs in the serializer. See EmbeddedRecordsMixin https://api.emberjs.com/ember-data/release/classes/EmbeddedRecordsMixin`, + false, + { + id: 'ds.model.embedded-option-in-belongs-to', + } + ); + } + } + }; + +export function makeDecorator(kind, descriptor) { + function relationship(type, options) { + if (typeof type === 'object') { + options = type; + type = undefined; + } + + if (kind !== 'attribute') { + assert( + `The first argument to a ${kind} must be a string representing a model type, not an instance of ${inspect( + type + )}. E.g., to define a relation to the Comment model, use ${kind}('comment')`, + typeof type === 'string' || typeof type === 'undefined' + ); + } + + options = options || {}; + + // Metadata about relationships is stored on the meta of + // the relationship. This is used for introspection and + // serialization. Note that `name` is populated lazily + // the first time the CP is called. + // `key` is a to-be deprecated alias to `name`. + const meta: PropertyMeta = { + type, + options, + kind: kind, + name: '', + key: '', + }; + const schemaKind = kind === 'attribute' ? 'attributes' : 'relationships'; + if (kind !== 'attribute') { + meta.isRelationship = true; + } else { + meta.isAttribute = true; + } + + function buildDescriptor(target, key) { + meta.name = key; + meta.key = key; + if (DEBUG && assertProps) { + assertProps(target.constructor, meta); + } + let schema = schemaCacheFor(target.constructor); + schema[schemaKind] = schema[schemaKind] || new Map(); + schema[schemaKind].set(key, meta); + const get = descriptor.getter(key, meta); + const set = descriptor.setter(key, meta); + + const desc = tagged(target, key, { + configurable: true, + enumerable: true, + get, + set, + }); + + return desc; + } + + // enable use with `Model.extend({})` syntax + _setClassicDecorator(buildDescriptor, true); + + buildDescriptor.meta = function() { + deprecate(`calling this is deprecated`, false, { + id: 'ember-data:computed-meta', + until: '4.3', + for: '@ember-data/model', + since: { + available: '3.28', + enabled: '4.1', + }, + }); + return meta; + }; + + return buildDescriptor; + } + + return computedMacroWithOptionalParams(relationship); +} diff --git a/packages/store/addon/-private/system/model/internal-model.ts b/packages/store/addon/-private/system/model/internal-model.ts index 855ae719777..92f97884422 100644 --- a/packages/store/addon/-private/system/model/internal-model.ts +++ b/packages/store/addon/-private/system/model/internal-model.ts @@ -6,7 +6,8 @@ import { get, notifyPropertyChange, set } from '@ember/object'; import { assign } from '@ember/polyfills'; import { _backburner as emberBackburner, cancel } from '@ember/runloop'; import { DEBUG } from '@glimmer/env'; -import { tracked } from '@glimmer/tracking'; +import { cached, tracked } from '@glimmer/tracking'; +import Ember from 'ember'; import RSVP, { Promise } from 'rsvp'; @@ -50,6 +51,7 @@ type PromiseManyArray = InstanceType>; declare _relationshipProxyCache: ConfidentDict; declare error: any; + declare currentState: any; constructor(public store: CoreStore | Store, public identifier: StableRecordIdentifier) { if (HAS_MODEL_PACKAGE) { @@ -220,9 +223,9 @@ export default class InternalModel { this._relationshipProxyCache = Object.create(null); this.references = Object.create(null); this._deferredTriggers = []; + this.currentState = RootState.empty; } - @tracked currentState: any = RootState.empty; /* A tag which when dirtied allows things tracking a record's ID to recompute. When we update this we must also flushSyncObservers @@ -355,7 +358,6 @@ export default class InternalModel { let createOptions: any = { store, _internalModel: this, - currentState: this.currentState, }; if (!REQUEST_SERVICE) { @@ -865,7 +867,7 @@ export default class InternalModel { setupData(data) { let changedKeys = this._recordData.pushData(data, this.hasRecord); if (this.hasRecord) { - this._record._notifyProperties(changedKeys); + _notifyProperties(this._record, changedKeys); } this.send('pushedData'); } @@ -992,7 +994,7 @@ export default class InternalModel { if (CUSTOM_MODEL_CLASS) { this.store._notificationManager.notify(this.identifier, 'relationships'); } else { - this._record.notifyHasManyAdded(key); + this._record.notifyPropertyChange(key); } } } @@ -1022,7 +1024,7 @@ export default class InternalModel { if (CUSTOM_MODEL_CLASS) { this.store._notificationManager.notify(this.identifier, 'relationships'); } else { - this._record.notifyBelongsToChange(key, this._record); + this._record.notifyPropertyChange(key); } } } @@ -1094,7 +1096,7 @@ export default class InternalModel { this.send('rolledBack'); if (this._record && dirtyKeys && dirtyKeys.length > 0) { - this._record._notifyProperties(dirtyKeys); + _notifyProperties(this._record, dirtyKeys); } } @@ -1154,8 +1156,7 @@ export default class InternalModel { this.currentState = state; if (this.hasRecord) { - set(this._record, 'currentState', state); - flushSyncObservers(); + this._record.notifyPropertyChange('currentState'); } for (i = 0, l = setups.length; i < l; i++) { @@ -1365,7 +1366,7 @@ export default class InternalModel { if (CUSTOM_MODEL_CLASS) { this.store._notificationManager.notify(this.identifier, 'attributes'); } else { - this._record._notifyProperties(changedKeys); + _notifyProperties(this._record, changedKeys); } } @@ -1570,3 +1571,16 @@ export function extractRecordDataFromRecord(recordOrPromiseRecord) { return recordDataFor(recordOrPromiseRecord); } + +function _notifyProperties(target, keys) { + // changeProperties defers notifications until after the delegate + // and protects with a try...finally block + // previously used begin...endPropertyChanges but this is private API + changeProperties(() => { + let key; + for (let i = 0, length = keys.length; i < length; i++) { + key = keys[i]; + target.notifyPropertyChange(key); + } + }); +} diff --git a/packages/store/types/@glimmer/tracking/index.d.ts b/packages/store/types/@glimmer/tracking/index.d.ts new file mode 100644 index 00000000000..68d2e1b3749 --- /dev/null +++ b/packages/store/types/@glimmer/tracking/index.d.ts @@ -0,0 +1,3 @@ +export { tracked } from '@glimmer/tracking'; + +export function cached(target: any, key: string, desc: PropertyDescriptor): void; diff --git a/packages/store/types/ember/index.d.ts b/packages/store/types/ember/index.d.ts index 0cac9e5580b..0bf165818be 100644 --- a/packages/store/types/ember/index.d.ts +++ b/packages/store/types/ember/index.d.ts @@ -2,3 +2,10 @@ export function run(callback: Function); export const ENV: { DS_WARN_ON_UNKNOWN_KEYS?: boolean; }; + +export function _setClassicDecorator( + descFn: (target: any, key: string, desc: PropertyDescriptor) => PropertyDescriptor, + isDesc: true +): void; + +export function changeProperties(fn: () => void): void; From 772ec1d6d36f1b0c2a2d5f1df09b3883147ac1c2 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 22 Apr 2021 15:27:29 -0700 Subject: [PATCH 2/3] almost there --- .../records/property-changes-test.js | 43 ++++-- .../records/relationship-changes-test.js | 145 ++++++++---------- packages/-ember-data/tests/unit/model-test.js | 1 - .../model/relationships/belongs-to-test.js | 94 ++---------- packages/model/addon/-private/model.js | 44 ++++-- packages/model/addon/-private/util.ts | 12 +- .../-private/relationships/state/has-many.ts | 19 ++- .../-private/system/model/internal-model.ts | 5 +- packages/store/types/ember/index.d.ts | 2 + .../qunit-asserts/assert-warning.ts | 17 +- 10 files changed, 178 insertions(+), 204 deletions(-) diff --git a/packages/-ember-data/tests/integration/records/property-changes-test.js b/packages/-ember-data/tests/integration/records/property-changes-test.js index d7e8b8ebc59..315ffed85b0 100644 --- a/packages/-ember-data/tests/integration/records/property-changes-test.js +++ b/packages/-ember-data/tests/integration/records/property-changes-test.js @@ -110,30 +110,34 @@ module('integration/records/property-changes - Property changes', function(hooks }); }); - test('Saving a record trigger observers for locally changed attributes with the same canonical value', function(assert) { - assert.expect(1); - var person; + test('Saving a record trigger observers for locally changed attributes with the same canonical value', async function(assert) { + assert.expect(3); let store = this.owner.lookup('service:store'); let adapter = store.adapterFor('application'); + let observerCount = 0; adapter.updateRecord = function(store, type, snapshot) { - return resolve({ data: { id: 'wat', type: 'person', attributes: { 'last-name': 'Katz' } } }); - }; - - run(function() { - store.push({ + return resolve({ data: { - type: 'person', id: 'wat', + type: 'person', attributes: { - firstName: 'Yehuda', - lastName: 'Katz', + 'last-name': 'Katz', }, }, }); - person = store.peekRecord('person', 'wat'); - person.set('lastName', 'Katz!'); + }; + + let person = store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz', + }, + }, }); person.addObserver('firstName', function() { @@ -141,12 +145,17 @@ module('integration/records/property-changes - Property changes', function(hooks }); person.addObserver('lastName', function() { - assert.ok(true, 'lastName observer should be triggered'); + observerCount++; }); - run(function() { - person.save(); - }); + person.set('lastName', 'Katz!'); + + assert.strictEqual(observerCount, 1, 'lastName observer should be triggered'); + + await person.save(); + + assert.strictEqual(observerCount, 2, 'lastName observer should be triggered'); + assert.strictEqual(person.lastName, 'Katz', 'We changed back to canonical'); }); test('store.push should not override a modified attribute', function(assert) { diff --git a/packages/-ember-data/tests/integration/records/relationship-changes-test.js b/packages/-ember-data/tests/integration/records/relationship-changes-test.js index 4ade1af4862..c322dfd77dd 100644 --- a/packages/-ember-data/tests/integration/records/relationship-changes-test.js +++ b/packages/-ember-data/tests/integration/records/relationship-changes-test.js @@ -1,6 +1,7 @@ import EmberObject, { get, set } from '@ember/object'; import { alias } from '@ember/object/computed'; import { run } from '@ember/runloop'; +import settled from '@ember/test-helpers/settled'; import { module, test } from 'qunit'; @@ -106,59 +107,51 @@ module('integration/records/relationship-changes - Relationship changes', functi this.owner.register('serializer:application', JSONAPISerializer.extend()); }); - test('Calling push with relationship triggers observers once if the relationship was empty and is added to', function(assert) { + test('Calling push with relationship triggers observers once if the relationship was empty and is added to', async function(assert) { assert.expect(1); let store = this.owner.lookup('service:store'); - let person = null; let observerCount = 0; - run(() => { - store.push({ - data: { - type: 'person', - id: 'wat', - attributes: { - firstName: 'Yehuda', - lastName: 'Katz', - }, - relationships: { - siblings: { - data: [], - }, + let person = store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz', + }, + relationships: { + siblings: { + data: [], }, }, - }); - person = store.peekRecord('person', 'wat'); + }, }); - run(() => { - person.addObserver('siblings.[]', function() { - observerCount++; - }); - // prime the pump - person.get('siblings'); + person.addObserver('siblings.[]', function() { + observerCount++; }); - run(() => { - store.push({ - data: { - type: 'person', - id: 'wat', - attributes: {}, - relationships: { - siblings: { - data: [sibling1Ref], - }, + // make sure the relationship has been accessed at least once so that things + // are materialized and notifications sent. + await person.get('siblings'); + + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: {}, + relationships: { + siblings: { + data: [sibling1Ref], }, }, - included: [sibling1], - }); + }, + included: [sibling1], }); - run(() => { - assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); - }); + assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); }); test('Calling push with relationship recalculates computed alias property if the relationship was empty and is added to', function(assert) { @@ -325,60 +318,56 @@ module('integration/records/relationship-changes - Relationship changes', functi }); }); - test('Calling push with relationship triggers observers once if the relationship was made shorter', function(assert) { - assert.expect(1); + test('Calling push with relationship triggers observers once if the relationship was made shorter', async function(assert) { + assert.expect(2); let store = this.owner.lookup('service:store'); - let person = null; let observerCount = 0; - run(() => { - store.push({ - data: { - type: 'person', - id: 'wat', - attributes: { - firstName: 'Yehuda', - lastName: 'Katz', - }, - relationships: { - siblings: { - data: [sibling1Ref], - }, + let person = store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz', + }, + relationships: { + siblings: { + data: [sibling1Ref], }, }, - included: [sibling1], - }); - person = store.peekRecord('person', 'wat'); + }, + included: [sibling1], }); - run(() => { - person.addObserver('siblings.[]', function() { - observerCount++; - }); - // prime the pump - person.get('siblings'); + person.addObserver('siblings.[]', function() { + observerCount++; }); - run(() => { - store.push({ - data: { - type: 'person', - id: 'wat', - attributes: {}, - relationships: { - siblings: { - data: [], - }, + // prime the pump + await person.get('siblings'); + + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: {}, + relationships: { + siblings: { + data: [], }, }, - included: [], - }); + }, + included: [], }); - run(() => { - assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); - }); + // canonical updates have a runloop to flush before ui notifications occur + await settled(); + + assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); + let siblings = await person.get('siblings'); + assert.strictEqual(siblings.length, 0, 'We removed the sibling'); }); test('Calling push with relationship triggers observers once if the relationship was reordered', function(assert) { diff --git a/packages/-ember-data/tests/unit/model-test.js b/packages/-ember-data/tests/unit/model-test.js index 64d95140e99..956e38ec153 100644 --- a/packages/-ember-data/tests/unit/model-test.js +++ b/packages/-ember-data/tests/unit/model-test.js @@ -772,7 +772,6 @@ module('unit/model - Model', function(hooks) { [prop]: DSattr('string'), name: DSattr('string'), }); - this.owner.register('model:native-post', NativePost); this.owner.register('model:legacy-post', LegacyPost); const msg = `'${prop}' is a reserved property name on instances of classes extending Model.`; diff --git a/packages/-ember-data/tests/unit/model/relationships/belongs-to-test.js b/packages/-ember-data/tests/unit/model/relationships/belongs-to-test.js index 6d4077e07b4..d66c6bec90d 100644 --- a/packages/-ember-data/tests/unit/model/relationships/belongs-to-test.js +++ b/packages/-ember-data/tests/unit/model/relationships/belongs-to-test.js @@ -726,6 +726,9 @@ module('unit/model/relationships - DS.belongsTo', function(hooks) { name: DS.attr('string'), hobby: DS.belongsTo('hobby', { serialize: true, async: true }), }); + Person.toString = () => { + return 'model:person'; + }; this.owner.register('model:hobby', Hobby); this.owner.register('model:person', Person); @@ -735,46 +738,9 @@ module('unit/model/relationships - DS.belongsTo', function(hooks) { adapter.shouldBackgroundReloadRecord = () => false; - run(() => { - store.push({ - data: [ - { - type: 'hobby', - id: '1', - attributes: { - name: 'fishing', - }, - }, - { - type: 'hobby', - id: '2', - attributes: { - name: 'coding', - }, - }, - { - type: 'person', - id: '1', - attributes: { - name: 'Tom Dale', - }, - relationships: { - hobby: { - data: { type: 'hobby', id: '1' }, - }, - }, - }, - ], - }); - }); - - return run(() => { - return store.findRecord('person', 1).then(person => { - assert.expectWarning(() => { - get(person, 'hobby'); - }, /You provided a serialize option on the "hobby" property in the "person" class, this belongs in the serializer. See Serializer and it's implementations/); - }); - }); + assert.expectWarning(() => { + store.modelFor('person'); + }, /You provided a serialize option on the "hobby" property in the "model:person" class, this belongs in the serializer. See Serializer and it's implementations/); }); testInDebug('belongsTo gives a warning when provided with an embedded option', function(assert) { @@ -786,55 +752,19 @@ module('unit/model/relationships - DS.belongsTo', function(hooks) { name: DS.attr('string'), hobby: DS.belongsTo('hobby', { embedded: true, async: true }), }); + Person.toString = () => { + return 'model:person'; + }; this.owner.register('model:hobby', Hobby); this.owner.register('model:person', Person); let store = this.owner.lookup('service:store'); let adapter = store.adapterFor('application'); - adapter.shouldBackgroundReloadRecord = () => false; - - run(() => { - store.push({ - data: [ - { - type: 'hobby', - id: '1', - attributes: { - name: 'fishing', - }, - }, - { - type: 'hobby', - id: '2', - attributes: { - name: 'coding', - }, - }, - { - type: 'person', - id: '1', - attributes: { - name: 'Tom Dale', - }, - relationships: { - hobby: { - data: { type: 'hobby', id: '1' }, - }, - }, - }, - ], - }); - }); - - return run(() => { - return store.findRecord('person', 1).then(person => { - assert.expectWarning(() => { - get(person, 'hobby'); - }, /You provided an embedded option on the "hobby" property in the "person" class, this belongs in the serializer. See EmbeddedRecordsMixin/); - }); - }); + assert.expectWarning(() => { + store.modelFor('person'); + }, /You provided an embedded option on the "hobby" property in the "model:person" class, this belongs in the serializer. See EmbeddedRecordsMixin/); }); test('belongsTo should be async by default', function(assert) { diff --git a/packages/model/addon/-private/model.js b/packages/model/addon/-private/model.js index e572877e092..31ae98f2994 100644 --- a/packages/model/addon/-private/model.js +++ b/packages/model/addon/-private/model.js @@ -1,6 +1,6 @@ import { assert, deprecate, warn } from '@ember/debug'; import EmberError from '@ember/error'; -import EmberObject, { computed } from '@ember/object'; +import EmberObject, { computed, notifyPropertyChange } from '@ember/object'; import { dependentKeyCompat } from '@ember/object/compat'; import { isNone } from '@ember/utils'; import { DEBUG } from '@glimmer/env'; @@ -27,6 +27,8 @@ import { import Errors from './errors'; import { relationshipFromMeta } from './system/relationships/relationship-meta'; +const { changeProperties, meta: metaFor } = Ember; + const DEBUG__STR = Date.now(); const RecordMeta = new WeakMap(); const SchemaCache = new WeakMap(); @@ -46,7 +48,7 @@ function cacheFor(map, key, debugProp) { return v; } -export function getRecordMeta(record) { +function getRecordMeta(record) { return cacheFor(RecordMeta, record, 'recordMeta'); } @@ -68,13 +70,18 @@ class Tag { this.rev++; } consume(v) { - this.dirty; + this.dirty; // subscribe this.isDirty = false; - this.value = v; + this.value = v; // set cached value } } -export function getTag(record, key: string) { +const STABLE_UNTRACKED_OBJ = {}; +function flushSyncObservers() { + notifyPropertyChange(STABLE_UNTRACKED_OBJ, '-tracking-prop'); +} + +function getTag(record, key) { const rm = getRecordMeta(record); rm.tags = rm.tags || {}; return (rm.tags[key] = rm.tags[key] || new Tag()); @@ -85,7 +92,8 @@ export function tagged(target, key, desc) { const setter = desc.set; desc.get = function() { let tag = getTag(this, key); - if (tag.isDirty && !this.isDestroyed && !this.isDestroying) { + if (tag.isDirty) { + // TODO it would be nice to no-op for && !this.isDestroyed && !this.isDestroying here tag.consume(getter.call(this)); } @@ -98,6 +106,12 @@ export function tagged(target, key, desc) { tag.notify(); }; } + dependentKeyCompat(desc); + // enable Model.reopen to overwrite an existing prop + desc.teardown = () => { + metaFor(target).removeDescriptors(key); + }; + metaFor(target).writeDescriptors(key, desc); return desc; } @@ -109,8 +123,6 @@ function getWithDefault(meta, prop, value) { return v; } -const { changeProperties } = Ember; - function isInvalidError(error) { return error && error.isAdapterError === true && error.code === 'InvalidError'; } @@ -403,10 +415,20 @@ class Model extends EmberObject { notifyPropertyChange(key, silent) { let meta = getRecordMeta(this); if (meta.tags && meta.tags[key]) { - if (!silent) { - meta.tags[key].notify(); + // notify the tracked world + meta.tags[key].notify(); + let propMeta = this.constructor.metaForProperty(key); + + // if we are doing a batch update (silent) or if the relationship has a PromiseProxy we must + // notify underlying chains the old fashioned way. + if (silent || (propMeta && propMeta.isRelationship && propMeta.options.async !== false)) { + super.notifyPropertyChange(key); + } else { + // notify old world of computeds and observers + flushSyncObservers(); } } else { + // this isn't a prop we manage so pass-thru super.notifyPropertyChange(key); } } @@ -2125,7 +2147,7 @@ class Model extends EmberObject { */ static toString() { let name = this.modelName; - if (!name && this.name !== 'Function') { + if (!name && this.name !== 'Function' && this.name !== 'Class') { name = this.name; } return `model:${name}`; diff --git a/packages/model/addon/-private/util.ts b/packages/model/addon/-private/util.ts index 3267d029c89..7b69aa3c8c8 100644 --- a/packages/model/addon/-private/util.ts +++ b/packages/model/addon/-private/util.ts @@ -46,9 +46,10 @@ interface PropertyMeta { kind: string; name: string; /** + * Alias of name * @deprecated */ - key: string; + key?: string; isRelationship?: true; isAttribute?: true; } @@ -57,7 +58,7 @@ const assertProps = DEBUG && function assertProps(target, meta) { const { name, kind, options } = meta; - if (['_internalModel', 'recordData', 'currentState'].indexOf(name) !== -1) { + if (['_internalModel', 'currentState'].indexOf(name) !== -1) { throw new Error( `'${name}' is a reserved property name on instances of classes extending Model. Please choose a different property name for your belongsTo on ${target.toString()}` ); @@ -93,7 +94,7 @@ export function makeDecorator(kind, descriptor) { if (kind !== 'attribute') { assert( - `The first argument to a ${kind} must be a string representing a model type, not an instance of ${inspect( + `The first argument to ${kind} must be a string representing a model type, not an instance of ${inspect( type )}. E.g., to define a relation to the Comment model, use ${kind}('comment')`, typeof type === 'string' || typeof type === 'undefined' @@ -112,7 +113,6 @@ export function makeDecorator(kind, descriptor) { options, kind: kind, name: '', - key: '', }; const schemaKind = kind === 'attribute' ? 'attributes' : 'relationships'; if (kind !== 'attribute') { @@ -123,7 +123,9 @@ export function makeDecorator(kind, descriptor) { function buildDescriptor(target, key) { meta.name = key; - meta.key = key; + if (meta.isRelationship) { + meta.key = key; + } if (DEBUG && assertProps) { assertProps(target.constructor, meta); } diff --git a/packages/record-data/addon/-private/relationships/state/has-many.ts b/packages/record-data/addon/-private/relationships/state/has-many.ts index 0273ac79869..0128020414b 100755 --- a/packages/record-data/addon/-private/relationships/state/has-many.ts +++ b/packages/record-data/addon/-private/relationships/state/has-many.ts @@ -114,6 +114,7 @@ export default class ManyRelationship extends Relationship { //a hack for not removing new records //TODO remove once we have proper diffing + let originalState = this.currentState.slice(); let newRecordDatas = this.currentState.filter( // only add new internalModels which are not yet in the canonical state of this // relationship (a new internalModel can be in the canonical state if it has @@ -129,10 +130,24 @@ export default class ManyRelationship extends Relationship { this._manyArray.flushCanonical(toSet); } */ + let stateDidChange = !(originalState.length === toSet.length); this.currentState = toSet; super.flushCanonical(); - // Once we clean up all the flushing, we will be left with at least the notifying part - this.notifyHasManyChange(); + + // avoid overly notifying if we didn't actually change anything. + if (!stateDidChange) { + for (let i = 0; i < originalState.length; i++) { + if (originalState[i] !== toSet[i]) { + stateDidChange = true; + break; + } + } + } + + if (stateDidChange) { + // Once we clean up all the flushing, we will be left with at least the notifying part + this.notifyHasManyChange(); + } } //TODO(Igor) idx not used currently, fix diff --git a/packages/store/addon/-private/system/model/internal-model.ts b/packages/store/addon/-private/system/model/internal-model.ts index 92f97884422..476a0dd19bc 100644 --- a/packages/store/addon/-private/system/model/internal-model.ts +++ b/packages/store/addon/-private/system/model/internal-model.ts @@ -6,7 +6,7 @@ import { get, notifyPropertyChange, set } from '@ember/object'; import { assign } from '@ember/polyfills'; import { _backburner as emberBackburner, cancel } from '@ember/runloop'; import { DEBUG } from '@glimmer/env'; -import { cached, tracked } from '@glimmer/tracking'; +import { tracked } from '@glimmer/tracking'; import Ember from 'ember'; import RSVP, { Promise } from 'rsvp'; @@ -1014,6 +1014,7 @@ export default class InternalModel { // // that said, also not clear why we haven't moved this to retainedmanyarray so maybe that's the bit that's just not working manyArray.retrieveLatest(); + this._record.notifyPropertyChange(key); } } } @@ -1580,7 +1581,7 @@ function _notifyProperties(target, keys) { let key; for (let i = 0, length = keys.length; i < length; i++) { key = keys[i]; - target.notifyPropertyChange(key); + target.notifyPropertyChange(key, true); } }); } diff --git a/packages/store/types/ember/index.d.ts b/packages/store/types/ember/index.d.ts index 0bf165818be..c7f2833fcd1 100644 --- a/packages/store/types/ember/index.d.ts +++ b/packages/store/types/ember/index.d.ts @@ -9,3 +9,5 @@ export function _setClassicDecorator( ): void; export function changeProperties(fn: () => void): void; + +export function meta(obj: any): any; diff --git a/packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-warning.ts b/packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-warning.ts index 9155a571c06..734f53ec922 100644 --- a/packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-warning.ts +++ b/packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-warning.ts @@ -58,23 +58,28 @@ function verifyWarning(config: WarningConfig, label?: string): AssertSomeResult } return isMatched; }); + // remove the handled warnings from WARNINGS_FOR_TEST WARNINGS_FOR_TEST = WARNINGS_FOR_TEST.filter(warning => { - matchedWarnings.indexOf(warning) === -1; + return matchedWarnings.indexOf(warning) === -1; }); HANDLED_WARNINGS_FOR_TEST.push(...matchedWarnings); let expectedCount = typeof config.count === 'number' ? config.count : 1; let passed = matchedWarnings.length === expectedCount; + let message = + label || + `Expected ${expectedCount} warning${expectedCount === 1 ? '' : 's'} for ${config.id} during test, ${ + passed ? expectedCount : 'but ' + matchedWarnings.length + } warnings were found.`; + if (!passed) { + message += `\n\nOther warnings found:\n\t${WARNINGS_FOR_TEST.map(w => w.message).join('\n\t')}`; + } return { result: passed, actual: { id: config.id, count: matchedWarnings.length }, expected: { id: config.id, count: expectedCount }, - message: - label || - `Expected ${expectedCount} warning${expectedCount === 1 ? '' : 's'} for ${config.id} during test, ${ - passed ? expectedCount : 'but ' + matchedWarnings.length - } warnings were found.`, + message, }; } From 18962a5045f677709dff663c7cc0c1665642c463 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 22 Apr 2021 18:23:44 -0700 Subject: [PATCH 3/3] stash --- .../tests/integration/relationships/one-to-one-test.js | 4 +++- packages/adapter/addon/index.ts | 2 +- packages/model/addon/-private/belongs-to.js | 2 ++ packages/model/addon/-private/model.js | 9 +++++++++ 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/-ember-data/tests/integration/relationships/one-to-one-test.js b/packages/-ember-data/tests/integration/relationships/one-to-one-test.js index c248570150b..30cc8fab43c 100644 --- a/packages/-ember-data/tests/integration/relationships/one-to-one-test.js +++ b/packages/-ember-data/tests/integration/relationships/one-to-one-test.js @@ -1,3 +1,4 @@ +import { set } from '@ember/object'; import { run } from '@ember/runloop'; import { module, test } from 'qunit'; @@ -583,7 +584,8 @@ module('integration/relationships/one_to_one_test - OneToOne relationships', fun assert.expectAssertion(function() { run(function() { - stanley.set('bestFriend', resolve(igor)); + // stanley.set('bestFriend', resolve(igor)); + set(stanley, 'bestFriend', resolve(igor)); }); }, /You passed in a promise that did not originate from an EmberData relationship. You can only pass promises that come from a belongsTo or hasMany relationship to the get call./); }); diff --git a/packages/adapter/addon/index.ts b/packages/adapter/addon/index.ts index c6e4134347a..b206082bb04 100644 --- a/packages/adapter/addon/index.ts +++ b/packages/adapter/addon/index.ts @@ -127,7 +127,7 @@ export default class Adapter extends EmberObject implements MinimumAdapterInterf */ findRecord(store: Store, type: ShimModelClass, id: string, snapshot: Snapshot): Promise { if (DEBUG) { - throw new Error('You subclassed the Adapter class but missing a findRecord override'); + throw new Error('You subclassed the Adapter class but are missing a findRecord override'); } return RSVPPromise.resolve(); diff --git a/packages/model/addon/-private/belongs-to.js b/packages/model/addon/-private/belongs-to.js index 9a2a2af6010..9479668b8e4 100644 --- a/packages/model/addon/-private/belongs-to.js +++ b/packages/model/addon/-private/belongs-to.js @@ -108,11 +108,13 @@ import { makeDecorator } from './util'; export default makeDecorator('belongsTo', { getter(key) { return function() { + debugger; return this._internalModel.getBelongsTo(key); }; }, setter(key) { return function(value) { + debugger; this._internalModel.setDirtyBelongsTo(key, value); }; }, diff --git a/packages/model/addon/-private/model.js b/packages/model/addon/-private/model.js index 31ae98f2994..ef79e9be095 100644 --- a/packages/model/addon/-private/model.js +++ b/packages/model/addon/-private/model.js @@ -433,6 +433,15 @@ class Model extends EmberObject { } } + set(key, value) { + // prevent _setProp within Ember.set from reading before writing. + // when obj.set is called directly + if (this.constructor.fields.has(key)) { + this[key] = value; + } + super.set(key, value); + } + /** If this property is `true` the record is in the `new` state. A record will be in the `new` state when it has been created on the