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

Support for Angular Signals ? #2088

Open
PowerKiKi opened this issue Nov 8, 2023 · 7 comments
Open

Support for Angular Signals ? #2088

PowerKiKi opened this issue Nov 8, 2023 · 7 comments
Labels

Comments

@PowerKiKi
Copy link
Collaborator

Angular v17 includes a stable API for Signals. I am wondering if that might have an impact for apollo-angular or not.

So far I would say that because apollo-angular is mostly doing XHR, and that XHR must be cancellable, then we must keep using observable. And if somebody would rather work with non-cancellable signal (which does not sound like a great idea to me), then it is up to them to adapt their consuming code. So there is nothing to be done within the lib.

But I'd like to hear the thoughts of the community and see if I am maybe missing something ?

@PowerKiKi PowerKiKi added the idea label Nov 8, 2023
@PowerKiKi PowerKiKi pinned this issue Nov 8, 2023
@JesseZomer
Copy link

Hello,

I agree that using observables in the library seems like the best plan. I do however have some questions about using apollo-angular together with signals. There are a few things that I'm running into that make it harder to work with than I want it to be, hopefully you have some ideas how to remedy these.

When I want to consume the query results in my component I use the provided toSignal from Angular. I don't want to check for null values everywhere, so I would like to use { requireSync: true } with an initialValue.

I tried using useInitialLoading: true but that isn't sync. That would also give me back data: Data | null which isn't really what I want.

Now I solve it by adding a startWith({data: initialData, loading: true, errors: undefined }) to the pipe. Before that I have a map operator that removes the queryname property from the data field

        map((res: ApolloQueryResult<T>) => ({
            ...res,
            data: res.data.queryname
        }))

because having that extra property only gets in the way (especially in the types, because its dynamic). But that's probably there to accommodate the __typename field I guess?

But having to do this for every query is a bit cumbersome. I would love for an initialValue option in (Watch)QueryOptions that would do the work of the startWith operator above.

Now it might seem a bit weird to have initial data when loading is true, but when you create a signal from your query, I would prefer to not have to null check every computed that is derived from that data. eg:

 query = toSignal(...);
 multiplied = computed(() => query().data.selection * 2

instead of

 query = toSignal(...)
 multiplied = computed(() => (query().data?.selection ?? 1) * 2)

Got any tips op workarounds for how best deal with these situations?

@PowerKiKi
Copy link
Collaborator Author

PowerKiKi commented Dec 11, 2023

For your use-case I think it would be possible to write your own function that wraps everything you need. Probably something similar to:

function apolloToSignal<T, Q extends keyof T, U>(
    query : Observable<ApolloQueryResult<T>, 
    queryName: Q, 
    initialValue: U
): Signal<T[Q] | U>

It's only pseudo-code, but it should be possible to make it work.

For the time being I'd probably avoid introducing signal-related things in this package, unless we have a clear and strong need for it.

@DaSchTour
Copy link

I'm not sure if that's a real use case or if that would be good architecture. But something like a combination of watchQuery and computed/effect could be helpful. This could look something like this.

query = signal('');
searchResult: Signal<ApolloResponse<SearchResult>> = this.apollo.signalQuery({query: this.query});

So the signalQuery would attach to the signal query and every new value to the query would set the variables on the watchQuery and this would respond with GraphQL response.

@rafagsiqueira
Copy link

I have an actual use case such as the one you describe. My component has an input signal, and the query uses that signal value on its variables. I am currently using an effect which calls fetch.
I would like to be able to use the signal on the query variables for a watchQuery, and to also be able to call refetch on the QueryRef.

@PowerKiKi
Copy link
Collaborator Author

PowerKiKi commented Apr 26, 2024

@DaSchTour,

every new value to the query would set the variables

I assume you meant "every new value to the variables" ? Because your goal is to react when variables change ? So your prototype would be more like:

const query = gql`query myQuery($myVar: Int)`;
const variables = signal({myVar: 123});
const searchResult: Signal<ApolloResponse<SearchResult>> = this.apollo.signalQuery({query: query, variables: variables});

variables.set({myVar: 456});

This kind of thing, watching variables, does not exist anymore in apollo-angular. It was removed before v1 landed. And the removal cause some trouble back then.

In our private projects, we use this kind of pattern a lot. It is super convenient to watch variables. So I wouldn't be against re-adding it. But the question is with what kind of API ?

Most often you need to debounce variables, otherwise there is a high chance you send too many XHR in a very short period of time. That sounds more like a job for Rxjs than for signals to me.

Also, this.apollo.signalQuery() means that an XHR will be sent immediately upon calling that function, because signals cannot be asynchronous. In some cases, where you already have the variables ready, it could be fine. But in our projects we very often setup the observable and subscribe later, when we have the needed variables and when other things around are setup. This would not work anymore.

Firing XHR too soon sounds like big pitfall to me. That's why I still cannot see how an integration with signals would improve the DX.

Also I think we would lose the ability to control when we unsubscribe. While unsubscribing when the component is destroyed can be ok in basic cases, there are a lot cases where things are not that straightforward. For instance a query inside a service would never be unsubscribed to. That sounds like a huge issue.

@rafagsiqueira,

If I guess your code correctly, that means an XHR is fired every time the variables changes, potentially extremely often, and the previous XHRs are never cancelled. That can quickly end up being a performance issue.

What I think is a more robust solution, and happens to be doable only with Rxjs AFAIK, is the following:

export class FooComponent {
    private readonly apollo = inject(Apollo);

    public readonly variables = input.required<UsersVariables>();

    public readonly result: Observable<ApolloQueryResult<Users>> = toObservable(this.variables).pipe(
        debounceTime(20), // Not too often
        switchMap( // Cancel previous XHR
            variables =>
                this.apollo.watchQuery<Users, UsersVariables>({
                    query: usersQuery,
                    variables: variables,
                }).valueChanges,
        ),
    );
}

I have been trying to reproduce something similar where this.result would be a Signal, but I actually couldn't. Even if given up the debounce and switchMap, I am not sure how to do it... Can you convert my sample into something that looks more like your code ?

tld;dr, I still thinks that signals are not the right tool for handling XHR, because the timing of XHR subscription, cancelling and unsubscription (from watching cache) are critical. And the whole reason for signal to exist is to be purely synchronous, and thus making it impossible to control the timing. Unless by doing hacks to re-expose things, but then you'd be re-implementing Rxjs while fighting signal nature.

@ChandlerBaskins
Copy link

Does it cancel XHR though? Looking at angular http client they implemented an abort controller when unsubscribing from the source observable. In query we call the Apollo client with a from promise.

@PowerKiKi
Copy link
Collaborator Author

Does it cancel XHR though?

Yes, apollo-angular will cancel XHR if you use our HttpLink, or our HttpBatchLink. And apollo-client will reject their promise if our link is erroring.

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

No branches or pull requests

5 participants