Add some timing tests for trackedFunction #1011

NullVoxPopuli · 2023-10-04T13:21:39Z

Investigation of #1010

stackblitz · 2023-10-04T13:21:41Z

Run & review this pull request in StackBlitz Codeflow.

changeset-bot · 2023-10-04T13:21:42Z

🦋 Changeset detected

Latest commit: c982307

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
ember-resources	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-10-04T13:24:25Z

Estimated impact to a consuming app, depending on which bundle is imported

	js	min	min + gzip	min + brotli
/index.js	26.22 kB	5.71 kB	2.1 kB	1.85 kB
├── core/class-based/index.js	9.39 kB	2.48 kB	1.16 kB	999 B
├── core/function-based/index.js	15.01 kB	4.12 kB	1.55 kB	1.35 kB
└── core/use.js	11.18 kB	3.46 kB	1.4 kB	1.22 kB
/link.js	2.67 kB	376 B	233 B	185 B
/service.js	20.59 kB	5.74 kB	2.12 kB	1.86 kB
/util/debounce.js	3.07 kB	861 B	447 B	365 B
/util/ember-concurrency.js	5.07 kB	1.59 kB	755 B	643 B
/util/fps.js	3.16 kB	919 B	478 B	386 B
/util/function.js	10.06 kB	2.77 kB	1.03 kB	900 B
/util/helper.js	2.12 kB	303 B	218 B	177 B
/util/keep-latest.js	2.08 kB	412 B	261 B	209 B
/util/map.js	5.95 kB	2.44 kB	1.1 kB	927 B
/util/remote-data.js	7.96 kB	2.37 kB	809 B	704 B

github-actions · 2023-10-04T13:25:46Z

Preview URLs

readme: https://693ebe38.ember-resources.pages.dev
api docs: https://693ebe38.ember-resources.pages.dev/modules.html

NullVoxPopuli · 2023-10-04T18:25:15Z

ember-resources/src/util/function.ts

+    // 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;


this is the fix

This makes a lot of sense. And these code comments are great

NullVoxPopuli · 2023-10-04T21:08:07Z

Blocked on actions/setup-node#865

Techn1x

Thanks a bunch. Tests are great too!

Techn1x · 2023-10-04T22:27:35Z

ember-resources/src/util/function.ts

+    // 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;


This makes a lot of sense. And these code comments are great

Techn1x · 2023-10-04T22:29:49Z

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';


Looks like this cell import is unused, probably leftover from investigation

thanks, yeah, I need to fix linting on this repo 🙃
there is a bug with prettier rn though, and I'm waiting for them to do another release

Techn1x · 2023-10-04T22:30:51Z

test-app/tests/utils/function/timing-test.gts

+      step(`fn:begin:${value}`);
+      await Promise.resolve();
+      step(`fn:end:${value}`);
+      return `yay:${value}`;


Techn1x · 2023-10-04T22:40:21Z

I'll find some time today to pnpm patch the fix in my app to test it out, but I'm pretty confident you nailed it

Techn1x · 2023-10-05T00:14:03Z

Seems that the fix line causes a tracking error 🤔

I can see the patch I added, so I know it's being applied

NullVoxPopuli · 2023-10-05T00:18:55Z

ooo this is good to catch before merging this -- what's the code that caused that?

Techn1x · 2023-10-05T00:23:12Z

The error points to line 188 in that minified source which looks like it's the if (this.data) this.data = null fix line that produces the error.

It seems to be on initial run. Looks like retry() function gets called twice for some reason

EDIT: I'm running Ember 4.12.3, and using this in a component - debugging shows directTrackedFunction is the one in use here

Techn1x · 2023-10-05T00:32:16Z

what's the code that caused that?

shortened a bit, but it's essentially this

class TimeOnTaskUsage extends Component<Signature> {
  @service studentEvents!: StudentEventsService

  fetchUsage = trackedFunction(this, () => {
    return this.studentEvents.timeOnTaskUsage( // returns a promise
      'writing_legends',
      this.args.statsScope,
      this.args.period,
    )
  })

  <template>
    {{#if this.fetchUsage.isPending}}
      loading!
    {{else if this.fetchUsage.isError}}
      error!
    {{else if this.fetchUsage.value}}
      {{log this.fetchUsage.value}}
    {{/if}}
  </template>
}

NullVoxPopuli · 2023-10-05T01:26:48Z

how are you causing the change to happen? statsScope changing? both args? something else?

thanks!

Techn1x · 2023-10-05T02:39:33Z

@tracked period = 'named-period:this-year' is a controller tracked query param that is passed down to this component
statsScope is a getter derived from a value provided by the route model

  get statsScope(): StatsScope {
    if (this.args.model.uiScope.scope === 'school') {
      return { scope: 'school', ids: [this.args.model.uiScope.id] }
    } else {
      ...
    }
  }

But I think that's a red herring - I can still repro the error even when the tracked values are removed from the function, since the error occurs on first run.

  fetchUsage = trackedFunction(this, () => {
    return this.studentEvents.timeOnTaskUsage( // returns a promise
      'writing_legends',
      { scope: 'school', ids: ['1'] }, // this.args.statsScope,
      'named-period:this-year', // this.args.period,
    )
  })

or even without any this usage

  fetchUsage = trackedFunction(this, async () => {
    return await new Promise((res) => setTimeout(res, 1000))
  })

The fact that your tests seem to work tells me that it's probably something unique to my setup, even though this is a fairly simple repro in my app :/

I will try to get this reproduced in some kind of playground

NullVoxPopuli · 2023-10-05T02:57:04Z

ya know, I get the error now, too.
This is good. It's all over my tests.

So I think I fell for the injected peerDependency problem again 😩
it seems my last push caused this, and the push before was fine. I've reverted that change now.

apologies for the run around.
it would have popped up when the setup-node released a new version -- but oofta.

Techn1x · 2023-10-05T03:10:11Z

apologies for the run around.

no trouble at all, I'm just glad to help! You do so much cool work!

I ended up coding up a potential repro in limber, though - but since it's using this addon before this PR fix, it doesn't show the error, but I was able to manually repro the bad behaviour by debugging in the console and running
if (this.data) this.data = null at the correct place in the minified code

(and sure enough, if I instead run this.data = null at the same place without the data read, the bad behaviour goes goes away)

not sure if this is the correct way to share, but it seems to share
https://limber.glimdown.com/edit?c=IYFxFMFsAcIEwAQCdzSQewegZgkTgBjAawEsA7AcwVIGdaBXcBACzGloC4B6by0kCwYAjAHSF0kbgDkGAGzkA1dAA8ACumjzS3KMPBIAtClroGSQuFrctC7gEYADPfsAoVwAMvlAFa0EcqQAbuCupDDoSCAIAMKS0Ojk4OTR2BiQCADkAAKUgZCQBtwSEUkpmQDcrrw0EVEIAN54BCTgiAC%2BCGmSWbn5hUjc%2BERkVJXu4Qn1TcOtcABiDOSEIKSJCJ3dGZl6BsZWZhZW3AyrctzYSytr5OOu4CpT0XDg2MDy0YRywPQIACJQTAPCDkOD%2BOKlZLRBquBBdcAgQgsACqtGAlGYAF5miM2otlqtEgAKQR0AA0CB%2BAE9lggiQBKBCYgB8jVhcOQCPM5EpAHdgAIEEleQg1Ok6OAiUSTIyWQhaAiACrhcBmEBS2WskwkpBMekUpyORz0%2Bns9r0qrsgA8EBg3wgzPZcIaDQAxKRcKTaKJsAikaj0eBRHQ1Mk4BRKO12k6OQAZADyAEE-gBJaQAcRjLvAcgVNE9LDoPr9KLRGODtAAokgMEgozG4ZWAEpN%2BNNrMNHN5j14Qve32I0uB0RBYByJj1jkcgDKyJiMUr0%2BnHe4HsnCCtQyg0Ht4Ed0a8HiAA&format=glimdown

Techn1x · 2023-10-05T03:15:07Z

confirmed the updated fix works in my app too 🚀

github-actions bot deployed to ember-resources (Preview) October 4, 2023 13:24 View deployment

github-actions bot deployed to ember-resources (Preview) October 4, 2023 18:20 View deployment

github-actions bot deployed to ember-resources (Preview) October 4, 2023 18:24 View deployment

NullVoxPopuli commented Oct 4, 2023

View reviewed changes

github-actions bot deployed to ember-resources (Preview) October 4, 2023 18:28 View deployment

Techn1x approved these changes Oct 4, 2023

View reviewed changes

github-actions bot deployed to ember-resources (Preview) October 5, 2023 02:59 View deployment

github-actions bot deployed to ember-resources (Preview) October 11, 2023 20:50 View deployment

NullVoxPopuli added 7 commits October 16, 2023 12:05

Add some timing tests for trackedFunction

e98c121

Update tests

5168dd4

Fix #1010

99e728e

Add changeset

606ba4b

Add a comment and don't nullify if we don't need to

407ccf7

Whoops

09bbad6

Don't read before set

8e79ff5

trigger ci

c982307

NullVoxPopuli force-pushed the trackedFunction-timing branch from 1c16752 to c982307 Compare October 16, 2023 16:05

github-actions bot deployed to ember-resources (Preview) October 16, 2023 16:11 View deployment

NullVoxPopuli merged commit c31054c into main Oct 16, 2023
27 checks passed

NullVoxPopuli deleted the trackedFunction-timing branch October 16, 2023 20:30

github-actions bot mentioned this pull request Oct 16, 2023

Release Preview #1015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some timing tests for trackedFunction #1011

Add some timing tests for trackedFunction #1011

NullVoxPopuli commented Oct 4, 2023

stackblitz bot commented Oct 4, 2023

changeset-bot bot commented Oct 4, 2023 •

edited

github-actions bot commented Oct 4, 2023 •

edited

github-actions bot commented Oct 4, 2023 •

edited

NullVoxPopuli Oct 4, 2023

Techn1x Oct 4, 2023

NullVoxPopuli commented Oct 4, 2023

Techn1x left a comment

Techn1x Oct 4, 2023

Techn1x Oct 4, 2023

NullVoxPopuli Oct 5, 2023

Techn1x Oct 4, 2023

Techn1x commented Oct 4, 2023

Techn1x commented Oct 5, 2023

NullVoxPopuli commented Oct 5, 2023

Techn1x commented Oct 5, 2023 •

edited

Techn1x commented Oct 5, 2023 •

edited

NullVoxPopuli commented Oct 5, 2023

Techn1x commented Oct 5, 2023 •

edited

NullVoxPopuli commented Oct 5, 2023

Techn1x commented Oct 5, 2023 •

edited

Techn1x commented Oct 5, 2023

Add some timing tests for trackedFunction #1011

Add some timing tests for trackedFunction #1011

Conversation

NullVoxPopuli commented Oct 4, 2023

stackblitz bot commented Oct 4, 2023

changeset-bot bot commented Oct 4, 2023 • edited

🦋 Changeset detected

github-actions bot commented Oct 4, 2023 • edited

github-actions bot commented Oct 4, 2023 • edited

Preview URLs

NullVoxPopuli Oct 4, 2023

Choose a reason for hiding this comment

Techn1x Oct 4, 2023

Choose a reason for hiding this comment

NullVoxPopuli commented Oct 4, 2023

Techn1x left a comment

Choose a reason for hiding this comment

Techn1x Oct 4, 2023

Choose a reason for hiding this comment

Techn1x Oct 4, 2023

Choose a reason for hiding this comment

NullVoxPopuli Oct 5, 2023

Choose a reason for hiding this comment

Techn1x Oct 4, 2023

Choose a reason for hiding this comment

Techn1x commented Oct 4, 2023

Techn1x commented Oct 5, 2023

NullVoxPopuli commented Oct 5, 2023

Techn1x commented Oct 5, 2023 • edited

Techn1x commented Oct 5, 2023 • edited

NullVoxPopuli commented Oct 5, 2023

Techn1x commented Oct 5, 2023 • edited

NullVoxPopuli commented Oct 5, 2023

Techn1x commented Oct 5, 2023 • edited

Techn1x commented Oct 5, 2023

changeset-bot bot commented Oct 4, 2023 •

edited

github-actions bot commented Oct 4, 2023 •

edited

github-actions bot commented Oct 4, 2023 •

edited

Techn1x commented Oct 5, 2023 •

edited

Techn1x commented Oct 5, 2023 •

edited

Techn1x commented Oct 5, 2023 •

edited

Techn1x commented Oct 5, 2023 •

edited