Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the implementation of trackedFunction such that loading/error state are revealed #363

Closed
NullVoxPopuli opened this issue Feb 1, 2022 · 9 comments · Fixed by #426
Labels

Comments

@NullVoxPopuli
Copy link
Owner

NullVoxPopuli commented Feb 1, 2022

Gonna copy @chriskrycho's https://v5.chriskrycho.com/journal/async-data-and-autotracking-in-ember-octane/
(but without the assertions for now, cause that's a breaking change).

userland API would be the same as trackedFunction is today, but in addition to .value, there would be .isPending, .isError and .error properties added

@chriskrycho
Copy link

Better yet, just use and re-export ember-async-data Instead of reimplementing. 😉

@NullVoxPopuli
Copy link
Owner Author

v2 addon soon?

@chriskrycho
Copy link

I would happily review and merge a PR converting it! I just don't have time right now to tackle that myself!

@NullVoxPopuli
Copy link
Owner Author

NullVoxPopuli commented Feb 1, 2022

excellent :D

I'm still trying to figure out how to properly wrangle TS + Rollup + addon-dev tho 😅

Link fof reference: https://github.com/chriskrycho/ember-async-data

@andreisebastianc-ccc
Copy link

Just want to confirm that this would allow me to easily test a component doing API request with useFunction:

new

{{#if this.dataset.isPending}}
    <LoadingIndicator data-test-loading-indicator />
{{else}}
    {{this.dataset.value}}
{{/if}}

existing

{{#if this.dataset.value}}
    {{this.dataset.value}}
{{else}}
    <LoadingIndicator data-test-loading-indicator />
{{/if}}

test

  test(
    'it displays a loading indicator while the dataset is loading',
    async function (assert) {
      this.server.timing = 5000; // exaggerated for observability
      await render(hbs`<DatasetInfo />`);
      assert
        .dom('[data-test-loading-indicator]')
        .exists('should render the loading indicator at component render');
    }
  );

With the current approach the test waits for the whole 5000ms before running the assertion.

@NullVoxPopuli
Copy link
Owner Author

It would, except i wan only considering this change for trackedFunction, as i believe it has better ergonomics

@andreisebastianc-ccc
Copy link

(I'm on 3.2.1)

How would you represent that there is an async operation to the user?

I'm curious if the behavior of useFunction i'm observing is what's expected. I wrote two tests, one that checks for a loading indicator and one that checks for the render of required values. The loading indicator test always fails because the assertion runs on the template only after there is a value. It's like there is a test waiter that make the assertions run only after the re-render.

It looks like the proper way of doing this is to rely on a Resource / useResource and have internal state. Would you agree?

I guess I could do this:

{{#if this.isLoading}}
  <LoadingIndicator />
{{/if}}
{{#if this.dataset.value}}
  {{this.dataset.value}}
{{/if}}
  @tracked isLoading = true;
  useFunction(() => {
    const response = await fetch(`/characters/`) ;
    this.isLoading = false;
    return response;
  })

but I have the impression that changing component tracked properties like this is not what one would want to do.

@NullVoxPopuli
Copy link
Owner Author

changing component tracked properties like this is not what one would want to do.

correct, this is a side effect, which can be pretty spooky beyond simple examples.

and, this is also why resources and derived state are such good patterns for encapsulating this kind of state.

How would you represent that there is an async operation to the user?

the way you were doing previously is sufficient for most cases.
Next time I find some time to work on ember-resources, I'm going to pull in @chriskrycho's library and that'll handle all the edge cases, error states, etc. (and will require another major version bump due to non-backwards compatible behavior -- which I can work around in v4, but unless a ton of people are using v4 (which I have no stats on), idk if it's worth it to absorb the API differences in a v4 minor? idk, open for discussion)).

It looks like the proper way of doing this is to rely on a Resource / useResource and have internal state. Would you agree?

yeah, most of what I like using those abstractions for is for building other resources that are nicer in actual app code.

  • Resource -- will be extended to load my specific data (or based on some convention, a kind of data, like what ember-data-resources does)
  • useResource - abstracted away behind another helper function that I usually don't use the word use in front of -- like, loadRecord or loadRecords, for example -- this pattern can provide defaults, runtime argument checking, etc

@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2022

🎉 This issue has been resolved in version 4.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants