query icon indicating copy to clipboard operation
query copied to clipboard

[angular-query] Accessing `.data` kicks off the `CreateQueryOptions` function

Open ekrapfl opened this issue 1 year ago • 3 comments

Describe the bug

I have a very simple use case when I do not have an error handler or loading indicator, so I am trying to create a query and save a reference to only the .data signal. I am not invoking the signal immediately, but saving a reference to only the signal. ie: something = injectQuery(() => { ... }).data;

I am using a signal input as a part of my query key.

I wanted to make that signal input required, but in doing so, Angular started throwing a runtime error saying that the required input had no value. From the outside, I was always passing a value in for the input.

After I reverted the input back to not required, I noticed that the CreateQueryOptions function I was using for the signal was actually being invoked immdeiately upon construction of the component as well as after the input value was set.

I was able to narrow it down and realize that this only happens when I have .data on the end in order to store off the signal. I was only doing this for the sake of simplicity. Nothing else in my code needs a reference to the query itself. They only need access to the data signal, so that's what I stored off. I was not expecting that to immediately invoke the options, and thus access the signal input before it is initialized.

I can confirm that if I store the reference to the entire query (ie: without .data), then it works correctly (and I can add .required if I want to). It confuses me because either way, my template will access the signal "immediately" via either query.data() or data(). But I suppose that is actually when rendering, vs at construct time when I store off .data.

Is this just bad practice the way I am trying to do it, or would this actually be considered a bug?

Your minimal, reproducible example

https://stackblitz.com/edit/angular-query-input-order?file=src%2Fapp%2Fsimple-example.component.ts&preset=node

Steps to reproduce

  • Open the repro
  • Look in the TanStack devtools and see that the "broken" example query ran twice. Once with null, and once the correct result: image
  • Now, change someInput to be required (see commented out line where the input is defined)
  • You will get a runtime error in the console like so: image

Expected behavior

I would expect that I can store off a reference to the .data signal whenever I want without it accessing signal inputs too soon.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • OS: MacOS 14.5
  • Browser: Edge
  • Version: 125.0.2535.67

Tanstack Query adapter

angular-query

TanStack Query version

5.40.0

TypeScript version

5.3.3

Additional context

No response

ekrapfl avatar May 28 '24 20:05 ekrapfl

This happens because when the instance of an Angular component is constructed inputs aren’t immediately available. If a required input is read when not available it will throw.

To support required inputs without having to use component lifecycle methods I put in a delay for when the callback is executed and so when signals in that function will be read. That will be either when one of the properties of the query object is accessed or on next tick. The idea is that when a property (signal) is accessed any required input signal should be available.

It may be possible to change this into when the signal function is executed instead of just the property being accessed but not sure yet if it’s worth it as a fix would likely add complexity. Anyone encountering this: please add a comment explaining under what circumstances and what the use case is.

arnoud-dv avatar Aug 21 '24 23:08 arnoud-dv

I am also facing this issue. Btw i have raised this concern in the first PR of angular query. Because the query functions is evaluated instantly this was always going to be an issue.

gmfun avatar Sep 10 '24 11:09 gmfun

@gmfun I think what you are referring to is something else. It's the nature of signals that they always have a value whereas observables can be cold and only have a value when subscribed to. It's no issue when not sharing query instances.

arnoud-dv avatar Sep 10 '24 12:09 arnoud-dv

@arnoud-dv I got the same error if I use a component with a required input and injectQuery with dependency on the required input inside ng-boostrap modal. See repo.

The setup is 3 components: The App component, Modal component and Test Component. The test component has a required input and injectQuery inside.

The App component spawns the Modal component using NgbModal from ng-boostrap. The Modal component uses the Test component in the markup.

I guess, the query somehow gets evaluated immediately and gets Error: NG0950: Input is required but no value is available yet.

SergeyFilenko avatar Dec 07 '24 01:12 SergeyFilenko

https://github.com/TanStack/query/blob/main/packages/angular-query-experimental/src/create-base-query.ts#L67

this is the issue

const defaultedOptionsSignal = computed(() => {
      const options = runInInjectionContext(injector, () => optionsFn())
      const defaultedOptions = queryClient.defaultQueryOptions(options)
      defaultedOptions._optimisticResults = 'optimistic'
      return defaultedOptions
    })

    const observer = new Observer<
      TQueryFnData,
      TError,
      TData,
      TQueryData,
      TQueryKey
    >(queryClient, defaultedOptionsSignal())

defaultedOptionsSignal() is being computed. and for input.required() this value is not yet available. I possible solution would be to pass an dummy defaultOptions here without reading optionsFn(), else we will have this issue

@gmfun I think what you are referring to is something else. It's the nature of signals that they always have a value whereas observables can be cold and only have a value when subscribed to. It's no issue when not sharing query instances.

@arnoud-dv this is incorrect for input.required(). If its value is read before the input is initalized, i.e. before ngOnInit this will give error

gmfun avatar Dec 07 '24 15:12 gmfun

defaultedOptionsSignal() is being computed. and for input.required() this value is not yet available. I possible solution would be to pass an dummy defaultOptions

Dummy value would result in an invalid query result. The solution in the PR that mentions this issue is a closure that returns a computed with the observer.

Also as it is there is already a solution to support required signals via lazy initialization through a Proxy object and I haven't run into problems myself. For example the Angular router example in the documentation uses a required signal. But the new solution is more robust I'd expect, I'll test it on the examples given here.

arnoud-dv avatar Dec 08 '24 00:12 arnoud-dv

 For example the Angular router example in the documentation uses a required signal.

Well, it actually made me wonder if this router example is affected the same way as ng-boostrap, unfortunately, it is. If you introduce a nested component and pass the signal received from the router into nested component and then use it inside query it throws an error. See stackblitz

SergeyFilenko avatar Dec 08 '24 05:12 SergeyFilenko

If you introduce a nested component and pass the signal received from the router into nested component and then use it inside query it throws an error.

Thanks, I just tried this with the new solution and it doesn't throw an error anymore.

arnoud-dv avatar Dec 08 '24 11:12 arnoud-dv

@arnoud-dv It seems that this PR might have created another issue. Disabling a query with a boolean signal and setting this signal inside mutation onSuccess function throws an error Image

@Component({
  selector: 'app-root',
  standalone: true,
  imports: [RouterOutlet],
  template: `
    @if (query.data(); as query) {
      {{ query }}
    }
    
    <button (click)="mutation.mutate()">Mutate</button>
  `,
})
export class AppComponent {
  isEnabled = signal(false);

  query = injectQuery(() => ({
    queryKey: ['user'],
    queryFn: () => Promise.resolve('Hello there'),
    enabled: this.isEnabled(),
  }))

  mutation = injectMutation(() => ({
    mutationFn: () => Promise.resolve(),
    onSuccess: () => this.isEnabled.set(true),
  }))
}

~can't reproduce it on Stackblitz, so it. might just be me doing something wrong. The code is quite simple~

Codesanbox minimal reproducible example

Version "@tanstack/angular-query-experimental": "5.62.3", doesn't have this issue

SergeyFilenko avatar Dec 09 '24 01:12 SergeyFilenko

Disabling a query with a boolean signal and setting this signal inside mutation onSuccess function throws an error

Thanks for testing and the repro! This is now fixed in v5.62.5

arnoud-dv avatar Dec 09 '24 10:12 arnoud-dv