NullVoxPopuli / ember-resources

An implementation of Resources. Supports ember 3.28+
https://github.com/NullVoxPopuli/ember-resources/blob/main/docs/docs/README.md
MIT License
91 stars 37 forks source link

Add some timing tests for trackedFunction #1011

Closed NullVoxPopuli closed 10 months ago

NullVoxPopuli commented 11 months ago

Investigation of https://github.com/NullVoxPopuli/ember-resources/issues/1010

stackblitz[bot] commented 11 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

changeset-bot[bot] commented 11 months ago

🦋 Changeset detected

Latest commit: c982307290b34082a95ba0f0b46323c781e7b7e8

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[bot] commented 11 months ago

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[bot] commented 11 months ago

Preview URLs

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

NullVoxPopuli commented 11 months ago

Blocked on https://github.com/actions/setup-node/pull/865

Techn1x commented 11 months ago

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 commented 11 months ago

Seems that the fix line causes a tracking error 🤔

Screenshot 2023-10-05 at 10 11 02 am

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

Screenshot 2023-10-05 at 10 13 11 am

NullVoxPopuli commented 11 months ago

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

Techn1x commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

thanks!

Techn1x commented 11 months ago

@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 commented 11 months ago

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 :weary: 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 commented 11 months ago

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 commented 11 months ago

confirmed the updated fix works in my app too 🚀