From e98c12119c79f3e550655883354ba45d2cd99de6 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 4 Oct 2023 09:21:20 -0400 Subject: [PATCH 1/8] Add some timing tests for trackedFunction --- .../tests/utils/function/rendering-test.gts | 2 +- test-app/tests/utils/function/timing-test.gts | 59 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 test-app/tests/utils/function/timing-test.gts diff --git a/test-app/tests/utils/function/rendering-test.gts b/test-app/tests/utils/function/rendering-test.gts index ef11d6a3a..c3e2bd4b9 100644 --- a/test-app/tests/utils/function/rendering-test.gts +++ b/test-app/tests/utils/function/rendering-test.gts @@ -7,7 +7,7 @@ import { module, test } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; import { setOwner } from '@ember/application'; -import { use, resource, resourceFactory } from 'ember-resources'; +import { cell, use, resource, resourceFactory } from 'ember-resources'; import { trackedFunction } from 'ember-resources/util/function'; const timeout = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); diff --git a/test-app/tests/utils/function/timing-test.gts b/test-app/tests/utils/function/timing-test.gts new file mode 100644 index 000000000..9bf52d0f1 --- /dev/null +++ b/test-app/tests/utils/function/timing-test.gts @@ -0,0 +1,59 @@ +import Component from '@glimmer/component'; +import { tracked } from '@glimmer/tracking'; +import { click, render, settled } from '@ember/test-helpers'; +// @ts-ignore +import { on } from '@ember/modifier'; +import { module, test } from 'qunit'; +import { setupRenderingTest } from 'ember-qunit'; +import { setOwner } from '@ember/application'; + +import { cell, use, resource, resourceFactory } from 'ember-resources'; +import { trackedFunction } from 'ember-resources/util/function'; + +const timeout = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +module('Utils | trackedFunction | timing', function (hooks) { + setupRenderingTest(hooks); + + test('With Argument', async function (assert) { + let step = (msg: string) => assert.step(msg); + + let state = cell(0); + + async function fn(value) { + step(`fn:begin:${value}`); + await Promise.resolve(); + step(`fn:end:${value}`); + return 'yay'; + } + + const WithArgument = resourceFactory(num => resource(({ use }) => { + return use(trackedFunction(() => fn(num))); + })); + + await render( + + ); + + assert.verifySteps([ + 'fn:begin:0', 'loading', 'fn:end:0', 'loaded' + ]); + + state.current = 1; + await settled(); + + assert.verifySteps([ + 'fn:begin:1', 'loading', 'fn:end:1', 'loaded' + ]); + }); +}); From 5168dd4ff8bacb4b094200759e80d8da21652040 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 4 Oct 2023 14:15:06 -0400 Subject: [PATCH 2/8] Update tests --- test-app/tests/utils/function/timing-test.gts | 66 +++++++++++++++++-- 1 file changed, 60 insertions(+), 6 deletions(-) diff --git a/test-app/tests/utils/function/timing-test.gts b/test-app/tests/utils/function/timing-test.gts index 9bf52d0f1..12b2fc914 100644 --- a/test-app/tests/utils/function/timing-test.gts +++ b/test-app/tests/utils/function/timing-test.gts @@ -1,4 +1,5 @@ import Component from '@glimmer/component'; +import { concat } from '@ember/helper'; import { tracked } from '@glimmer/tracking'; import { click, render, settled } from '@ember/test-helpers'; // @ts-ignore @@ -24,7 +25,7 @@ module('Utils | trackedFunction | timing', function (hooks) { step(`fn:begin:${value}`); await Promise.resolve(); step(`fn:end:${value}`); - return 'yay'; + return `yay:${value}`; } const WithArgument = resourceFactory(num => resource(({ use }) => { @@ -36,24 +37,77 @@ module('Utils | trackedFunction | timing', function (hooks) { {{#let (WithArgument state.current) as |request|}} {{#if request.isLoading}} {{step 'loading'}} - {{else if request.isError}} + {{/if}} + {{#if request.isError}} {{step 'error'}} - {{else if request.value}} - {{step 'loaded'}} + {{/if}} + {{#if request.value}} + {{step (concat 'loaded:' request.value)}} {{/if}} {{/let}} ); assert.verifySteps([ - 'fn:begin:0', 'loading', 'fn:end:0', 'loaded' + 'fn:begin:0', 'loading', 'fn:end:0', 'loaded:yay:0' + ]); + + state.current = 1; + await settled(); + + assert.verifySteps([ + 'fn:begin:1', 'loading', 'fn:end:1', 'loaded:yay:1' + ]); + }); + + test('From a component class', async function (assert) { + let step = (msg: string) => assert.step(msg); + + let state = cell(0); + + class Example extends Component<{ Args: { value: unknown } }> { + request = trackedFunction(this, async () => { + let value = this.args.value; + step(`fn:begin:${value}`); + await Promise.resolve(); + step(`fn:end:${value}`); + return `yay:${value}`; + }); + + + } + + await render( + + ); + + assert.verifySteps([ + 'fn:begin:0', 'pending', 'loading', 'fn:end:0', 'loaded:yay:0', 'finished' ]); state.current = 1; await settled(); assert.verifySteps([ - 'fn:begin:1', 'loading', 'fn:end:1', 'loaded' + 'fn:begin:1', 'pending', 'loading', 'fn:end:1', 'loaded:yay:1', 'finished' ]); }); }); From 99e728ef37f2ab3ae946a807054fb7435207f025 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 4 Oct 2023 14:15:23 -0400 Subject: [PATCH 3/8] Fix #1010 --- ember-resources/src/util/function.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/ember-resources/src/util/function.ts b/ember-resources/src/util/function.ts index bf1a1d1d0..be4f34105 100644 --- a/ember-resources/src/util/function.ts +++ b/ember-resources/src/util/function.ts @@ -272,6 +272,7 @@ export class State { retry = async () => { if (isDestroyed(this) || isDestroying(this)) return; + this.data = null; // We need to invoke this before going async so that tracked properties are consumed (entangled with) synchronously this.promise = this.#fn(); From 606ba4b892037469fa85fe636f93aa988b5d5ab2 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 4 Oct 2023 14:17:06 -0400 Subject: [PATCH 4/8] Add changeset --- .changeset/neat-apples-shout.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/neat-apples-shout.md diff --git a/.changeset/neat-apples-shout.md b/.changeset/neat-apples-shout.md new file mode 100644 index 000000000..63972609f --- /dev/null +++ b/.changeset/neat-apples-shout.md @@ -0,0 +1,7 @@ +--- +"ember-resources": patch +--- + +`trackedFunction`: Fix timing issue where updating tracked data consumed in `trackedFunction` would not re-cause the `isLoading` state to become `true` again. + +Resolves #1010 From 407ccf7b06ddd298156ba09ea366f364cd493299 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 4 Oct 2023 14:20:37 -0400 Subject: [PATCH 5/8] Add a comment and don't nullify if we don't need to --- ember-resources/src/util/function.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ember-resources/src/util/function.ts b/ember-resources/src/util/function.ts index be4f34105..56dc2afa0 100644 --- a/ember-resources/src/util/function.ts +++ b/ember-resources/src/util/function.ts @@ -272,7 +272,12 @@ export class State { retry = async () => { if (isDestroyed(this) || isDestroying(this)) return; - this.data = null; + // We've previously had data, but we're about to run-again. + // we need to do this again so `isLoading` goes back to `true` when re-running. + if (this.data) { + this.data = null; + } + // We need to invoke this before going async so that tracked properties are consumed (entangled with) synchronously this.promise = this.#fn(); From 09bbad67915556df7400bb42105f2dd90fd8ba6b Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 4 Oct 2023 14:24:37 -0400 Subject: [PATCH 6/8] Whoops --- test-app/tests/utils/function/timing-test.gts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/test-app/tests/utils/function/timing-test.gts b/test-app/tests/utils/function/timing-test.gts index 12b2fc914..fcd477927 100644 --- a/test-app/tests/utils/function/timing-test.gts +++ b/test-app/tests/utils/function/timing-test.gts @@ -1,18 +1,12 @@ import Component from '@glimmer/component'; import { concat } from '@ember/helper'; -import { tracked } from '@glimmer/tracking'; -import { click, render, settled } from '@ember/test-helpers'; -// @ts-ignore -import { on } from '@ember/modifier'; +import { render, settled } from '@ember/test-helpers'; import { module, test } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; -import { setOwner } from '@ember/application'; -import { cell, use, resource, resourceFactory } from 'ember-resources'; +import { cell, resource, resourceFactory } from 'ember-resources'; import { trackedFunction } from 'ember-resources/util/function'; -const timeout = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); - module('Utils | trackedFunction | timing', function (hooks) { setupRenderingTest(hooks); @@ -21,7 +15,7 @@ module('Utils | trackedFunction | timing', function (hooks) { let state = cell(0); - async function fn(value) { + async function fn(value: number) { step(`fn:begin:${value}`); await Promise.resolve(); step(`fn:end:${value}`); @@ -29,7 +23,11 @@ module('Utils | trackedFunction | timing', function (hooks) { } const WithArgument = resourceFactory(num => resource(({ use }) => { - return use(trackedFunction(() => fn(num))); + let reactive = use(trackedFunction(() => fn(num))); + + // TODO: the types should allow us to directly return the use, + // but they don't currently + return () => reactive.current; })); await render( From 8e79ff5d571f58d570eeb85fabea565a79282ac7 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 4 Oct 2023 22:56:55 -0400 Subject: [PATCH 7/8] Don't read before set --- ember-resources/src/util/function.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ember-resources/src/util/function.ts b/ember-resources/src/util/function.ts index 56dc2afa0..897b35536 100644 --- a/ember-resources/src/util/function.ts +++ b/ember-resources/src/util/function.ts @@ -274,9 +274,10 @@ export class State { // We've previously had data, but we're about to run-again. // we need to do this again so `isLoading` goes back to `true` when re-running. - if (this.data) { - this.data = null; - } + // NOTE: we want to do this _even_ if this.data is already null. + // it's all in the same tracking frame and the important thing is taht + // we can't *read* data here. + this.data = null; // We need to invoke this before going async so that tracked properties are consumed (entangled with) synchronously this.promise = this.#fn(); From c982307290b34082a95ba0f0b46323c781e7b7e8 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 11 Oct 2023 16:46:58 -0400 Subject: [PATCH 8/8] trigger ci