From 0d782dfa92878a1c114f3a236a2f7883950238a1 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 13 Oct 2022 14:53:59 -0700 Subject: [PATCH 1/8] npm ignore publish for yarn-error.log --- packages/-ember-data/.npmignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/-ember-data/.npmignore b/packages/-ember-data/.npmignore index f081b33ade6..d4910ea2c6f 100644 --- a/packages/-ember-data/.npmignore +++ b/packages/-ember-data/.npmignore @@ -4,6 +4,7 @@ /tmp/ /types/ **/*.d.ts +yarn-error.log # dependencies /bower_components/ @@ -42,4 +43,3 @@ lib/scripts # whitelist yuidoc's data.json for api docs generation !/dist/docs/data.json - From 08d6d435f94affd7bfb0d74af33594ed93e0ea41 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 13 Oct 2022 14:54:14 -0700 Subject: [PATCH 2/8] dont wrap in deprecation proxy in prod builds --- packages/model/addon/-private/deprecated-promise-proxy.ts | 4 ++++ packages/store/addon/-private/proxies/promise-proxies.ts | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/packages/model/addon/-private/deprecated-promise-proxy.ts b/packages/model/addon/-private/deprecated-promise-proxy.ts index 15f4143a10b..364c4fc8c9c 100644 --- a/packages/model/addon/-private/deprecated-promise-proxy.ts +++ b/packages/model/addon/-private/deprecated-promise-proxy.ts @@ -1,4 +1,5 @@ import { deprecate } from '@ember/debug'; +import { DEBUG } from '@glimmer/env'; import { resolve } from 'rsvp'; @@ -16,6 +17,9 @@ const PROXIED_OBJECT_PROPS = ['content', 'isPending', 'isSettled', 'isRejected', export function deprecatedPromiseObject(promise: Promise): PromiseObject { const promiseObjectProxy: PromiseObject = promiseObject(promise); + if (!DEBUG) { + return promiseObjectProxy; + } const handler = { get(target: object, prop: string, receiver?: object): unknown { if (typeof prop === 'symbol') { diff --git a/packages/store/addon/-private/proxies/promise-proxies.ts b/packages/store/addon/-private/proxies/promise-proxies.ts index 7cbce4a4137..ad35d79fcd2 100644 --- a/packages/store/addon/-private/proxies/promise-proxies.ts +++ b/packages/store/addon/-private/proxies/promise-proxies.ts @@ -1,6 +1,7 @@ import { deprecate } from '@ember/debug'; import type ComputedProperty from '@ember/object/computed'; import { reads } from '@ember/object/computed'; +import { DEBUG } from '@glimmer/env'; import { resolve } from 'rsvp'; @@ -162,6 +163,9 @@ export function promiseArray>(promise: Promise export function promiseObject(promise: Promise): PromiseObjectProxy { const promiseObjectProxy: PromiseObjectProxy = _promiseObject(promise); + if (!DEBUG) { + return promiseObjectProxy; + } const handler = { get(target: object, prop: string, receiver?: object): unknown { if (typeof prop === 'symbol') { From e467bd695ecca4b0b56a706a55e2e4171fc17b2d Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 13 Oct 2022 15:11:51 -0700 Subject: [PATCH 3/8] add failing test to replicate bug --- .../acceptance/tracking-promise-flags-test.js | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js diff --git a/packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js b/packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js new file mode 100644 index 00000000000..f0d5930a580 --- /dev/null +++ b/packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js @@ -0,0 +1,66 @@ +import { render, settled } from '@ember/test-helpers'; + +import hbs from 'htmlbars-inline-precompile'; +import { module, test } from 'qunit'; + +import { setupRenderingTest } from 'ember-qunit'; + +import JSONAPIAdapter from '@ember-data/adapter/json-api'; +import Model, { attr } from '@ember-data/model'; +import Store from '@ember-data/store'; + +class Widget extends Model { + @attr name; +} + +module('acceptance/tracking-promise-flags', function (hooks) { + setupRenderingTest(hooks); + + hooks.beforeEach(function () { + let { owner } = this; + owner.register('service:store', Store); + owner.register('model:widget', Widget); + owner.register( + 'serializer:application', + class { + normalizeResponse = (_, __, data) => data; + static create() { + return new this(); + } + } + ); + }); + + test('can track isPending', async function (assert) { + const { owner } = this; + let resolve; + class TestAdapter extends JSONAPIAdapter { + findRecord() { + return new Promise((r) => { + resolve = r; + }); + } + } + owner.register('adapter:application', TestAdapter); + let store = owner.lookup('service:store'); + store.DISABLE_WAITER = true; + this.model = store.findRecord('widget', '1'); + + await render(hbs`{{#if this.model.isPending}}Pending{{else}}{{this.model.name}}{{/if}}`); + + assert.dom().containsText('Pending'); + + resolve({ + data: { + id: '1', + type: 'widget', + attributes: { + name: 'Contraption', + }, + }, + }); + await settled(); + + assert.dom().containsText('Contraption'); + }); +}); From 2bd896c80bb681183c3ab4bd3b96ff401c94d426 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 13 Oct 2022 17:12:56 -0700 Subject: [PATCH 4/8] add fix --- .../acceptance/tracking-promise-flags-test.js | 59 ++++++++++--------- .../-private/deprecated-promise-proxy.ts | 31 +++++++--- .../addon/-private/proxies/promise-proxies.ts | 23 ++++++-- 3 files changed, 72 insertions(+), 41 deletions(-) diff --git a/packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js b/packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js index f0d5930a580..9603d27e2b5 100644 --- a/packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js +++ b/packages/-ember-data/tests/acceptance/tracking-promise-flags-test.js @@ -1,13 +1,14 @@ import { render, settled } from '@ember/test-helpers'; import hbs from 'htmlbars-inline-precompile'; -import { module, test } from 'qunit'; +import { module } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; import JSONAPIAdapter from '@ember-data/adapter/json-api'; import Model, { attr } from '@ember-data/model'; import Store from '@ember-data/store'; +import { deprecatedTest } from '@ember-data/unpublished-test-infra/test-support/deprecated-test'; class Widget extends Model { @attr name; @@ -31,36 +32,40 @@ module('acceptance/tracking-promise-flags', function (hooks) { ); }); - test('can track isPending', async function (assert) { - const { owner } = this; - let resolve; - class TestAdapter extends JSONAPIAdapter { - findRecord() { - return new Promise((r) => { - resolve = r; - }); + deprecatedTest( + 'can track isPending', + { id: 'ember-data:deprecate-promise-proxies', until: '5.0', count: 6 }, + async function (assert) { + const { owner } = this; + let resolve; + class TestAdapter extends JSONAPIAdapter { + findRecord() { + return new Promise((r) => { + resolve = r; + }); + } } - } - owner.register('adapter:application', TestAdapter); - let store = owner.lookup('service:store'); - store.DISABLE_WAITER = true; - this.model = store.findRecord('widget', '1'); + owner.register('adapter:application', TestAdapter); + let store = owner.lookup('service:store'); + store.DISABLE_WAITER = true; + this.model = store.findRecord('widget', '1'); - await render(hbs`{{#if this.model.isPending}}Pending{{else}}{{this.model.name}}{{/if}}`); + await render(hbs`{{#if this.model.isPending}}Pending{{else}}{{this.model.name}}{{/if}}`); - assert.dom().containsText('Pending'); + assert.dom().containsText('Pending'); - resolve({ - data: { - id: '1', - type: 'widget', - attributes: { - name: 'Contraption', + resolve({ + data: { + id: '1', + type: 'widget', + attributes: { + name: 'Contraption', + }, }, - }, - }); - await settled(); + }); + await settled(); - assert.dom().containsText('Contraption'); - }); + assert.dom().containsText('Contraption'); + } + ); }); diff --git a/packages/model/addon/-private/deprecated-promise-proxy.ts b/packages/model/addon/-private/deprecated-promise-proxy.ts index 364c4fc8c9c..5a0b4fb40af 100644 --- a/packages/model/addon/-private/deprecated-promise-proxy.ts +++ b/packages/model/addon/-private/deprecated-promise-proxy.ts @@ -1,4 +1,5 @@ import { deprecate } from '@ember/debug'; +import { get } from '@ember/object'; import { DEBUG } from '@glimmer/env'; import { resolve } from 'rsvp'; @@ -15,6 +16,8 @@ function promiseObject(promise: Promise): PromiseObject { const ALLOWABLE_METHODS = ['constructor', 'then', 'catch', 'finally']; const PROXIED_OBJECT_PROPS = ['content', 'isPending', 'isSettled', 'isRejected', 'isFulfilled', 'promise', 'reason']; +const ProxySymbolString = String(Symbol.for('PROXY_CONTENT')); + export function deprecatedPromiseObject(promise: Promise): PromiseObject { const promiseObjectProxy: PromiseObject = promiseObject(promise); if (!DEBUG) { @@ -23,31 +26,41 @@ export function deprecatedPromiseObject(promise: Promise): PromiseObject unknown).bind(target); } - const value: unknown = target[prop]; - if (value && typeof value === 'function' && typeof value.bind === 'function') { - return value.bind(target); + if (PROXIED_OBJECT_PROPS.includes(prop)) { + return target[prop]; } - if (PROXIED_OBJECT_PROPS.includes(prop)) { - return value; + const value: unknown = get(target, prop); + if (value && typeof value === 'function' && typeof value.bind === 'function') { + return value.bind(receiver); } return undefined; diff --git a/packages/store/addon/-private/proxies/promise-proxies.ts b/packages/store/addon/-private/proxies/promise-proxies.ts index ad35d79fcd2..6796fb98817 100644 --- a/packages/store/addon/-private/proxies/promise-proxies.ts +++ b/packages/store/addon/-private/proxies/promise-proxies.ts @@ -1,4 +1,5 @@ import { deprecate } from '@ember/debug'; +import { get } from '@ember/object'; import type ComputedProperty from '@ember/object/computed'; import { reads } from '@ember/object/computed'; import { DEBUG } from '@glimmer/env'; @@ -161,6 +162,8 @@ export function promiseArray>(promise: Promise return new Proxy(promiseObjectProxy, handler); } +const ProxySymbolString = String(Symbol.for('PROXY_CONTENT')); + export function promiseObject(promise: Promise): PromiseObjectProxy { const promiseObjectProxy: PromiseObjectProxy = _promiseObject(promise); if (!DEBUG) { @@ -169,8 +172,16 @@ export function promiseObject(promise: Promise): PromiseObjectProxy { const handler = { get(target: object, prop: string, receiver?: object): unknown { if (typeof prop === 'symbol') { + if (String(prop) === ProxySymbolString) { + return; + } return Reflect.get(target, prop, receiver); } + + if (prop === 'constructor') { + return target.constructor; + } + if (!ALLOWABLE_METHODS.includes(prop)) { deprecate( `Accessing ${prop} on this PromiseObject is deprecated. The return type is being changed from PromiseObject to a Promise. The only available methods to access on this promise are .then, .catch and .finally`, @@ -185,15 +196,17 @@ export function promiseObject(promise: Promise): PromiseObjectProxy { }, } ); + } else { + return (target[prop] as () => unknown).bind(target); } - const value: unknown = target[prop]; - if (value && typeof value === 'function' && typeof value.bind === 'function') { - return value.bind(target); + if (PROXIED_OBJECT_PROPS.includes(prop)) { + return target[prop]; } - if (PROXIED_OBJECT_PROPS.includes(prop)) { - return value; + const value: unknown = get(target, prop); + if (value && typeof value === 'function' && typeof value.bind === 'function') { + return value.bind(receiver); } return undefined; From bc6a9b93fef0e079fcd299214616e02b07973994 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 13 Oct 2022 17:14:43 -0700 Subject: [PATCH 5/8] restore original deprecation --- packages/model/addon/-private/deprecated-promise-proxy.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/model/addon/-private/deprecated-promise-proxy.ts b/packages/model/addon/-private/deprecated-promise-proxy.ts index 5a0b4fb40af..4932f655ff2 100644 --- a/packages/model/addon/-private/deprecated-promise-proxy.ts +++ b/packages/model/addon/-private/deprecated-promise-proxy.ts @@ -38,15 +38,15 @@ export function deprecatedPromiseObject(promise: Promise): PromiseObject Date: Thu, 13 Oct 2022 17:50:02 -0700 Subject: [PATCH 6/8] cleanup --- .../tests/integration/records/save-test.js | 11 +---------- .../model/addon/-private/deprecated-promise-proxy.ts | 2 +- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/-ember-data/tests/integration/records/save-test.js b/packages/-ember-data/tests/integration/records/save-test.js index 59569fee7a8..ccadca3dc82 100644 --- a/packages/-ember-data/tests/integration/records/save-test.js +++ b/packages/-ember-data/tests/integration/records/save-test.js @@ -47,15 +47,6 @@ module('integration/records/save - Save Record', function (hooks) { assert.ok(true, 'save operation was resolved'); if (DEPRECATE_SAVE_PROMISE_ACCESS) { assert.strictEqual(saved.get('id'), '123'); - if (DEBUG) { - try { - saved.id; - assert.ok(false, 'access should error with .get assertion'); - } catch { - assert.ok(true, 'access errored correctly'); - } - } - assert.strictEqual(model.id, '123'); } else { assert.strictEqual(saved.id, undefined); @@ -69,7 +60,7 @@ module('integration/records/save - Save Record', function (hooks) { assert.strictEqual(saved.__ec_cancel__, undefined); assert.strictEqual(model.__ec_cancel__, undefined); - assert.expectDeprecation({ id: 'ember-data:model-save-promise', count: 4 }); + assert.expectDeprecation({ id: 'ember-data:model-save-promise', count: 11 }); } }); diff --git a/packages/model/addon/-private/deprecated-promise-proxy.ts b/packages/model/addon/-private/deprecated-promise-proxy.ts index 4932f655ff2..49e28182b29 100644 --- a/packages/model/addon/-private/deprecated-promise-proxy.ts +++ b/packages/model/addon/-private/deprecated-promise-proxy.ts @@ -38,7 +38,7 @@ export function deprecatedPromiseObject(promise: Promise): PromiseObject Date: Thu, 13 Oct 2022 17:54:24 -0700 Subject: [PATCH 7/8] more cleanup --- packages/-ember-data/tests/integration/records/save-test.js | 1 - packages/store/addon/-private/proxies/promise-proxies.ts | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/-ember-data/tests/integration/records/save-test.js b/packages/-ember-data/tests/integration/records/save-test.js index ccadca3dc82..ca0a5ba434b 100644 --- a/packages/-ember-data/tests/integration/records/save-test.js +++ b/packages/-ember-data/tests/integration/records/save-test.js @@ -1,5 +1,4 @@ import { run } from '@ember/runloop'; -import { DEBUG } from '@glimmer/env'; import { module, test } from 'qunit'; import { defer, reject, resolve } from 'rsvp'; diff --git a/packages/store/addon/-private/proxies/promise-proxies.ts b/packages/store/addon/-private/proxies/promise-proxies.ts index 6796fb98817..184b1c22f1e 100644 --- a/packages/store/addon/-private/proxies/promise-proxies.ts +++ b/packages/store/addon/-private/proxies/promise-proxies.ts @@ -125,6 +125,9 @@ const PROXIED_OBJECT_PROPS = ['content', 'isPending', 'isSettled', 'isRejected', export function promiseArray>(promise: Promise): PromiseArray { const promiseObjectProxy: PromiseArray = _promiseArray(promise); + if (!DEBUG) { + return promiseObjectProxy; + } const handler = { get(target: object, prop: string, receiver?: object): unknown { if (typeof prop === 'symbol') { From 027da7c2aee9ff9fd22ae73e3f5916d2ec7900d8 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 13 Oct 2022 22:52:05 -0700 Subject: [PATCH 8/8] dont deprecate for ec, fix prod test --- .../tests/integration/records/save-test.js | 20 +++++++++++-------- .../-private/deprecated-promise-proxy.ts | 7 ++++++- .../addon/-private/proxies/promise-proxies.ts | 12 +++++++++-- 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/packages/-ember-data/tests/integration/records/save-test.js b/packages/-ember-data/tests/integration/records/save-test.js index ca0a5ba434b..96f76976fef 100644 --- a/packages/-ember-data/tests/integration/records/save-test.js +++ b/packages/-ember-data/tests/integration/records/save-test.js @@ -38,28 +38,32 @@ module('integration/records/save - Save Record', function (hooks) { if (DEPRECATE_SAVE_PROMISE_ACCESS) { // `save` returns a PromiseObject which allows to call get on it - assert.strictEqual(saved.get('id'), undefined); + assert.strictEqual(saved.get('id'), undefined, `.get('id') is undefined before save resolves`); } deferred.resolve({ data: { id: '123', type: 'post' } }); let model = await saved; assert.ok(true, 'save operation was resolved'); if (DEPRECATE_SAVE_PROMISE_ACCESS) { - assert.strictEqual(saved.get('id'), '123'); - assert.strictEqual(model.id, '123'); + assert.strictEqual(saved.get('id'), '123', `.get('id') is '123' after save resolves`); + assert.strictEqual(model.id, '123', `record.id is '123' after save resolves`); } else { - assert.strictEqual(saved.id, undefined); - assert.strictEqual(model.id, '123'); + assert.strictEqual(saved.id, undefined, `.id is undefined after save resolves`); + assert.strictEqual(model.id, '123', `record.id is '123' after save resolves`); } assert.strictEqual(model, post, 'resolves with the model'); if (DEPRECATE_SAVE_PROMISE_ACCESS) { // We don't care about the exact value of the property, but accessing it // should not throw an error and only show a deprecation. saved.__ec_cancel__ = true; - assert.strictEqual(saved.__ec_cancel__, undefined); - assert.strictEqual(model.__ec_cancel__, undefined); + assert.true(saved.__ec_cancel__, '__ec_cancel__ can be accessed on the proxy'); + assert.strictEqual( + model.__ec_cancel__, + undefined, + '__ec_cancel__ can be accessed on the record but is not present' + ); - assert.expectDeprecation({ id: 'ember-data:model-save-promise', count: 11 }); + assert.expectDeprecation({ id: 'ember-data:model-save-promise', count: 10 }); } }); diff --git a/packages/model/addon/-private/deprecated-promise-proxy.ts b/packages/model/addon/-private/deprecated-promise-proxy.ts index 49e28182b29..2276c0849f9 100644 --- a/packages/model/addon/-private/deprecated-promise-proxy.ts +++ b/packages/model/addon/-private/deprecated-promise-proxy.ts @@ -14,6 +14,7 @@ function promiseObject(promise: Promise): PromiseObject { // constructor is accessed in some internals but not including it in the copyright for the deprecation const ALLOWABLE_METHODS = ['constructor', 'then', 'catch', 'finally']; +const ALLOWABLE_PROPS = ['__ec_yieldable__', '__ec_cancel__']; const PROXIED_OBJECT_PROPS = ['content', 'isPending', 'isSettled', 'isRejected', 'isFulfilled', 'promise', 'reason']; const ProxySymbolString = String(Symbol.for('PROXY_CONTENT')); @@ -24,7 +25,7 @@ export function deprecatedPromiseObject(promise: Promise): PromiseObject(promise: Promise): PromiseObject>(promise: Promise, labe // constructor is accessed in some internals but not including it in the copyright for the deprecation const ALLOWABLE_METHODS = ['constructor', 'then', 'catch', 'finally']; +const ALLOWABLE_PROPS = ['__ec_yieldable__', '__ec_cancel__']; const PROXIED_ARRAY_PROPS = [ 'length', '[]', @@ -129,10 +130,13 @@ export function promiseArray>(promise: Promise return promiseObjectProxy; } const handler = { - get(target: object, prop: string, receiver?: object): unknown { + get(target: object, prop: string, receiver: object): unknown { if (typeof prop === 'symbol') { return Reflect.get(target, prop, receiver); } + if (ALLOWABLE_PROPS.includes(prop)) { + return receiver[prop]; + } if (!ALLOWABLE_METHODS.includes(prop)) { deprecate( `Accessing ${prop} on this PromiseArray is deprecated. The return type is being changed from PromiseArray to a Promise. The only available methods to access on this promise are .then, .catch and .finally`, @@ -173,7 +177,7 @@ export function promiseObject(promise: Promise): PromiseObjectProxy { return promiseObjectProxy; } const handler = { - get(target: object, prop: string, receiver?: object): unknown { + get(target: object, prop: string, receiver: object): unknown { if (typeof prop === 'symbol') { if (String(prop) === ProxySymbolString) { return; @@ -185,6 +189,10 @@ export function promiseObject(promise: Promise): PromiseObjectProxy { return target.constructor; } + if (ALLOWABLE_PROPS.includes(prop)) { + return receiver[prop]; + } + if (!ALLOWABLE_METHODS.includes(prop)) { deprecate( `Accessing ${prop} on this PromiseObject is deprecated. The return type is being changed from PromiseObject to a Promise. The only available methods to access on this promise are .then, .catch and .finally`,