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(signals): Add refetch functionality to intersected results #163

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

Zacherl
Copy link
Contributor

@Zacherl Zacherl commented Mar 1, 2024

This change ensures that all intersected queries are refetched when "refetch" is called on the combined result.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Intersected results cannot be refetched via the refetch callback (it is undefined).

Issue Number: #162

A refetch callback is added that refetches all intersected queries when called.

Does this PR introduce a breaking change?

  • Yes
  • No

This change ensures that all intersected queries are refetched when "refetch" is called on the combined result.
Copy link

stackblitz bot commented Mar 1, 2024

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

@NetanelBasal
Copy link
Member

It will be a good idea to align the observable API and add the refetch also there

@NetanelBasal
Copy link
Member

We can even create a IntersectResult class and use it in both versions.

@Zacherl
Copy link
Contributor Author

Zacherl commented Mar 1, 2024

Thank you for your comments - I will be sure to address them.

@Zacherl
Copy link
Contributor Author

Zacherl commented Mar 4, 2024

I think a common class to use in both versions is not possible since the result types differ.

For intersectResults, the all property in the result type (Signal<QueryObserverResult<R> & { all: T }>) is either an array or a dictionary of signals. For intersectResults$, the all property is either an array or a dictionary of query observer results. If I wanted to unify it, it would be a breaking change.

Furthermore, I am not sure if it's very practical to unify both functions. If we use intersectResults$ as a base, we could extract this part as common code (again, breaking change for the interface):

function intersectResultValues<
  T extends Array<QueryObserverResult<any>> | Record<string, QueryObserverResult<any>>,
  R,
>(values: T, mapFn: (values: UnifiedTypes<T>) => R): QueryObserverResult<R> & { all: T } {
    const isArray = Array.isArray(values);
    const toArray = isArray ? values : Object.values(values);
    const refetch = () => Promise.all(toArray.map(v => v().refetch()));

    const mappedResult = {
      all: values,
      isSuccess: toArray.every((v) => v.isSuccess),
      isPending: toArray.some((v) => v.isPending),
      isLoading: toArray.some((v) => v.isLoading),
      isError: toArray.some((v) => v.isError),
      isFetching: toArray.some((v) => v.isFetching),
      error: toArray.find((v) => v.isError)?.error,
      data: undefined,
      refetch,
    } as unknown as QueryObserverResult<R> & { all: T };

    if (mappedResult.isSuccess) {
      if (isArray) {
        mappedResult.data = mapFn(
          toArray.map((r) => r.data) as UnifiedTypes<T>,
        );
      } else {
        const data = Object.entries(values).reduce((acc, [key, value]) => {
          acc[key as keyof UnifiedTypes<T>] = value.data;

          return acc;
        }, {} as UnifiedTypes<T>);

        mappedResult.data = mapFn(data);
      }
    }

    return mappedResult;
  }

We could then call this code from the signal version intersectResults by mapping the array/dictionary of signals to an array/dictionary of values.

As far as I can see, in this variant we also cannot reuse the "refetch" method reference, so we create it any time the values change. Actually, I think that we could only reuse the method reference in the signal variant, not in the observable operator.

Am I missing something?

@NetanelBasal
Copy link
Member

Ok, not critical.

@Zacherl
Copy link
Contributor Author

Zacherl commented Mar 4, 2024

Another thing I realized: Currently the data is only returned if all queries are successful. However, if I am not wrong, queries may return data even if they have an error (if data from a previous fetch is cached). Would it be feasible to map the data if all queries return something instead of only mapping it if all queries are successful?

@NetanelBasal
Copy link
Member

We could do it optionally by passing an explicitly option, but I'd do it in a different PR.

@NetanelBasal NetanelBasal merged commit 5a910cc into ngneat:main Mar 5, 2024
1 check passed
@Zacherl Zacherl deleted the patch-1 branch March 6, 2024 07:36
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.

3 participants