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.

Sign up for GitHub

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
Open

feat: optimistic updates #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions apps/example-app/src/app/crud/child.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import { Person } from './models';
providers: provideQueryState(DataService, {
name: ChildComponent.name,
query: 'queryOne',
cacheTime: 0,
cacheTime: 5000,
revalidateTriggers: false,
}),
})
Expand All @@ -64,13 +64,21 @@ export class ChildComponent implements OnInit {
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;
tapState(
{
onSuccess: (person) => {
this.model.id = person.id;
this.model.firstname = person.firstname;
this.model.lastname = person.lastname;
},
onError: () => {
console.log('some error');
},
},
})
{
optimisticUpdates: true,
}
)
)
);
}
Expand Down
20 changes: 17 additions & 3 deletions libs/query-state/src/lib/operators/tap-state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
import { MonoTypeOperatorFunction, tap } from 'rxjs';
import { QueryStateData } from '../contracts';

export function tapState<QueryData>(callbacks: {
type CallBackFunctions<T> = {
onError?: (error: unknown) => void;
onIdle?: () => void;
onLoading?: () => void;
onRevalidate?: () => void;
onSuccess?: (data: QueryData) => void;
}): MonoTypeOperatorFunction<QueryStateData<QueryData>> {
onSuccess?: (data: T) => void;
};

type CallBackOptions = {
optimisticUpdates?: boolean;
};

export function tapState<QueryData>(
callbacks: CallBackFunctions<QueryData>,
options?: CallBackOptions
): MonoTypeOperatorFunction<QueryStateData<QueryData>> {
return tap((data) => {
switch (data.state) {
case 'error':
Expand All @@ -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 •
edited
Loading

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 :)

callbacks.onSuccess?.(data.data as QueryData);
}

break;

case 'success':
Expand Down