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

feat: optimistic updates #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

salmin89
Copy link

@salmin89 salmin89 commented May 4, 2022

this will allow trigger onSuccess during revalidation. Let me know what you think

@@ -24,6 +33,11 @@ export function tapState<QueryData>(callbacks: {

case 'revalidate':
callbacks.onRevalidate?.();

if (options?.optimisticUpdates) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably missing something but how does this "enable" optimistic updates?

Copy link
Author

@salmin89 salmin89 May 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a recording of what the desired result is. For this gif, I have added an updated field in the data.service, to illustrate that once the api call completes, you will see the real updates.

 delay(1000),
      map((person) => {
        person.updatedAt = Date.now().toString();
        return person;
      })

Kapture 2022-05-08 at 17 49 44

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is "just" a refresh?
Optimistic updates are used when data is being updated and you already want to reflect the changes to the user, without waiting for the server response.
See the docs for react-query https://react-query.tanstack.com/guides/optimistic-updates

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, while the data was being revalidated, we didn't have a way to show any previous values.
See the before:
https://cdn.discordapp.com/attachments/962720938774921247/971513436569755688/Kapture_2022-05-04_at_16.45.17.gif

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I don't know if optimisticUpdates is the correct name then.
What about preload?
Could you also add a test for this please.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m ok with renaming the setting! Preload sounds a lot like prefetch. But I do like your first suggestion, refresh. Maybe backgroundRefresh?

Should the setting be able to be globally set to a default?

I will add tests and update with new examples once things are decided

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, after giving it some more thought I think we can make this possible without an additional config flag.
The data should be passed to the revalidate method of tapState.
The component code then looks like this if you want to use this future.
Notice the extra if statement because person will be undefined the first time because it isn't in the cache.

  ngOnInit(): void {
    this.queryState.effect(
      this.queryState.data$.pipe(
        tapState({
          onSuccess: (person) => {
            this.model.id = person.id;
            this.model.firstname = person.firstname;
            this.model.lastname = person.lastname;
          },
+          onRevalidate: (person) => {
+            if (person) {
+              this.model.id = person.id;
+              this.model.firstname = person.firstname;
+              this.model.lastname = person.lastname;
+            }
          },
        })
      )
    );
  }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned that solution initially on our discord conversation, but I think that the downside of this is that you will have to wire up all your components on both onSuccess, onRevalidate and onError, etc.

What I was thinking is that we can with some configuration set some of these things up so that we get the desired output with less boiler plate need. Like shown in this PR.

Anyone who wants custom handlers for each tap, can have that as well.

but these are just thoughts. I don’t think strongly about one over the other, so I’ll leave it up to you to decide :)

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

Successfully merging this pull request may close these issues.

2 participants