Skip to content

Commit

Permalink
address feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
runspired committed Aug 17, 2018
1 parent 957dd61 commit 6401c44
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 49 deletions.
28 changes: 15 additions & 13 deletions addon/-legacy-private/system/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,19 +220,19 @@ Store = Service.extend({
this._serializerCache = Object.create(null);

if (DEBUG) {
if (this._shouldTrackAsyncRequests === undefined) {
this._shouldTrackAsyncRequests = true;
if (this.shouldTrackAsyncRequests === undefined) {
this.shouldTrackAsyncRequests = false;
}
if (this._generateStackTracesForTrackedRequests === undefined) {
this._generateStackTracesForTrackedRequests = false;
if (this.generateStackTracesForTrackedRequests === undefined) {
this.generateStackTracesForTrackedRequests = false;
}

this._trackedAsyncRequests = [];
this._trackAsyncRequestStart = label => {
let trace =
'set `store._generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated';
'set `store.generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated';

if (this._generateStackTracesForTrackedRequests) {
if (this.generateStackTracesForTrackedRequests) {
try {
throw new Error(`EmberData TrackedRequest: ${label}`);
} catch (e) {
Expand All @@ -251,13 +251,15 @@ Store = Service.extend({
this._trackAsyncRequestEnd = token => {
let index = this._trackedAsyncRequests.indexOf(token);

if (index !== -1) {
this._trackedAsyncRequests.splice(index, 1);
if (index === -1) {
throw new Error(`Attempted to end tracking for the following request but it was not being tracked:\n${token}`);
}

this._trackedAsyncRequests.splice(index, 1);
};

this.__asyncWaiter = () => {
let shouldTrack = this._shouldTrackAsyncRequests;
let shouldTrack = this.shouldTrackAsyncRequests;
let tracked = this._trackedAsyncRequests;
let isSettled = tracked.length === 0;

Expand Down Expand Up @@ -873,7 +875,7 @@ Store = Service.extend({
};

if (DEBUG) {
if (this._generateStackTracesForTrackedRequests === true) {
if (this.generateStackTracesForTrackedRequests === true) {
let trace;

try {
Expand Down Expand Up @@ -2897,19 +2899,19 @@ Store = Service.extend({

if (DEBUG) {
Ember.Test.unregisterWaiter(this.__asyncWaiter);
let shouldTrack = this._shouldTrackAsyncRequests;
let shouldTrack = this.shouldTrackAsyncRequests;
let tracked = this._trackedAsyncRequests;
let isSettled = tracked.length === 0;

if (!isSettled) {
if (shouldTrack) {
throw new Error(
'Async Request leaks detected. Add a breakpoint here and set `store._generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
tracked.map(o => o.label).join('\n\t - ')
);
} else {
warn(
'Async Request leaks detected. Add a breakpoint here and set `store._generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
tracked.map(o => o.label).join('\n\t - '),
false,
{
Expand Down
22 changes: 11 additions & 11 deletions addon/-record-data-private/system/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,19 +226,19 @@ Store = Service.extend({
this.modelDataWrapper = new ModelDataWrapper(this);

if (DEBUG) {
if (this._shouldTrackAsyncRequests === undefined) {
this._shouldTrackAsyncRequests = true;
if (this.shouldTrackAsyncRequests === undefined) {
this.shouldTrackAsyncRequests = false;
}
if (this._generateStackTracesForTrackedRequests === undefined) {
this._generateStackTracesForTrackedRequests = false;
if (this.generateStackTracesForTrackedRequests === undefined) {
this.generateStackTracesForTrackedRequests = false;
}

this._trackedAsyncRequests = [];
this._trackAsyncRequestStart = label => {
let trace =
'set `store._generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated';
'set `store.generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated';

if (this._generateStackTracesForTrackedRequests) {
if (this.generateStackTracesForTrackedRequests) {
try {
throw new Error(`EmberData TrackedRequest: ${label}`);
} catch (e) {
Expand All @@ -263,7 +263,7 @@ Store = Service.extend({
};

this.__asyncWaiter = () => {
let shouldTrack = this._shouldTrackAsyncRequests;
let shouldTrack = this.shouldTrackAsyncRequests;
let tracked = this._trackedAsyncRequests;
let isSettled = tracked.length === 0;

Expand Down Expand Up @@ -882,7 +882,7 @@ Store = Service.extend({
};

if (DEBUG) {
if (this._generateStackTracesForTrackedRequests === true) {
if (this.generateStackTracesForTrackedRequests === true) {
let trace;

try {
Expand Down Expand Up @@ -3119,19 +3119,19 @@ Store = Service.extend({

if (DEBUG) {
Ember.Test.unregisterWaiter(this.__asyncWaiter);
let shouldTrack = this._shouldTrackAsyncRequests;
let shouldTrack = this.shouldTrackAsyncRequests;
let tracked = this._trackedAsyncRequests;
let isSettled = tracked.length === 0;

if (!isSettled) {
if (shouldTrack) {
throw new Error(
'Async Request leaks detected. Add a breakpoint here and set `store._generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
tracked.map(o => o.label).join('\n\t - ')
);
} else {
warn(
'Async Request leaks detected. Add a breakpoint here and set `store._generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
tracked.map(o => o.label).join('\n\t - '),
false,
{
Expand Down
1 change: 1 addition & 0 deletions tests/integration/store-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ test('find calls do not resolve when the store is destroyed', async function(ass

initializeStore(TestAdapter);

store.shouldTrackAsyncRequests = true;
store.push = function() {
assert('The test should have destroyed the store by now', store.get('isDestroyed'));

Expand Down
47 changes: 22 additions & 25 deletions tests/unit/store/async-leak-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,20 @@ module('unit/store async-waiter and leak detection', function(hooks) {
})
);
store = owner.lookup('service:store');
store.shouldTrackAsyncRequests = true;
});

test('the waiter properly waits for pending requests', async function(assert) {
let stepResolve;
let stepPromise = new Promise(resolveStep => {
stepResolve = resolveStep;
let findRecordWasInvoked;
let findRecordWasInvokedPromise = new Promise(resolveStep => {
findRecordWasInvoked = resolveStep;
});
this.owner.register(
'adapter:application',
JSONAPIAdapter.extend({
findRecord() {
return new Promise(resolve => {
stepResolve();
findRecordWasInvoked();

setTimeout(() => {
resolve({ data: { type: 'person', id: '1' } });
Expand All @@ -62,7 +63,7 @@ module('unit/store async-waiter and leak detection', function(hooks) {
'We return true when no requests have been initiated yet (pending queue flush is async)'
);

await stepPromise;
await findRecordWasInvokedPromise;

assert.equal(waiter(), false, 'We return false to keep waiting while requests are pending');

Expand All @@ -72,16 +73,16 @@ module('unit/store async-waiter and leak detection', function(hooks) {
});

test('waiter can be turned off', async function(assert) {
let stepResolve;
let stepPromise = new Promise(resolveStep => {
stepResolve = resolveStep;
let findRecordWasInvoked;
let findRecordWasInvokedPromise = new Promise(resolveStep => {
findRecordWasInvoked = resolveStep;
});
this.owner.register(
'adapter:application',
JSONAPIAdapter.extend({
findRecord() {
return new Promise(resolve => {
stepResolve();
findRecordWasInvoked();

setTimeout(() => {
resolve({ data: { type: 'person', id: '1' } });
Expand All @@ -92,7 +93,7 @@ module('unit/store async-waiter and leak detection', function(hooks) {
);

// turn off the waiter
store._shouldTrackAsyncRequests = false;
store.shouldTrackAsyncRequests = false;

let request = store.findRecord('person', '1');
let waiter = store.__asyncWaiter;
Expand All @@ -103,7 +104,7 @@ module('unit/store async-waiter and leak detection', function(hooks) {
'We return true when no requests have been initiated yet (pending queue flush is async)'
);

await stepPromise;
await findRecordWasInvokedPromise;

assert.equal(
store._trackedAsyncRequests.length,
Expand All @@ -119,16 +120,16 @@ module('unit/store async-waiter and leak detection', function(hooks) {

test('waiter works even when the adapter rejects', async function(assert) {
assert.expect(4);
let stepResolve;
let stepPromise = new Promise(resolveStep => {
stepResolve = resolveStep;
let findRecordWasInvoked;
let findRecordWasInvokedPromise = new Promise(resolveStep => {
findRecordWasInvoked = resolveStep;
});
this.owner.register(
'adapter:application',
JSONAPIAdapter.extend({
findRecord() {
return new Promise((resolve, reject) => {
stepResolve();
findRecordWasInvoked();

setTimeout(() => {
reject({ errors: [] });
Expand All @@ -147,13 +148,11 @@ module('unit/store async-waiter and leak detection', function(hooks) {
'We return true when no requests have been initiated yet (pending queue flush is async)'
);

await stepPromise;
await findRecordWasInvokedPromise;

assert.equal(waiter(), false, 'We return false to keep waiting while requests are pending');

await request.catch(e => {
assert.ok(true, 'promise was rejected');
});
await assert.rejects(request);

assert.equal(waiter(), true, 'We return true to end waiting when no requests are pending');
});
Expand Down Expand Up @@ -183,9 +182,7 @@ module('unit/store async-waiter and leak detection', function(hooks) {
'We return true when no requests have been initiated yet (pending queue flush is async)'
);

await request.catch(e => {
assert.ok(true, 'promise was rejected');
});
await assert.rejects(request);

assert.equal(waiter(), true, 'We return true to end waiting when no requests are pending');
});
Expand Down Expand Up @@ -257,7 +254,7 @@ module('unit/store async-waiter and leak detection', function(hooks) {
);

// turn off the waiter
store._shouldTrackAsyncRequests = false;
store.shouldTrackAsyncRequests = false;

store.findRecord('person', '1');
let waiter = store.__asyncWaiter;
Expand Down Expand Up @@ -321,15 +318,15 @@ module('unit/store async-waiter and leak detection', function(hooks) {
assert.equal(waiter(), false, 'We return false to keep waiting while requests are pending');
assert.equal(
store._trackedAsyncRequests[0].trace,
'set `store._generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated',
'set `store.generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated',
'We provide a useful default message in place of a trace'
);

await request;

assert.equal(waiter(), true, 'We return true to end waiting when no requests are pending');

store._generateStackTracesForTrackedRequests = true;
store.generateStackTracesForTrackedRequests = true;
request = store.findRecord('person', '2');

assert.equal(
Expand Down

0 comments on commit 6401c44

Please sign in to comment.