feat(angular-query-experimental): run query options callbacks in injection context

[ShacharHarshuv]

This is to allow consumers to use inject inside callbacks like queryFn, initialData etc.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
query	⬜️ Ignored (Inspect)	Visit Preview		May 1, 2024 9:30pm

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2d5a3df. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

🟥 Failed Commands
nx affected --targets=test:format,test:sherif,test:knip,test:eslint,test:lib,test:types,test:build,build,test:attw --parallel=3

---

Sent with 💌 from NxCloud.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2d5a3df:

Sandbox	Source
@tanstack/query-example-angular-basic	Configuration
@tanstack/query-example-react-basic-typescript	Configuration
@tanstack/query-example-solid-basic-typescript	Configuration
@tanstack/query-example-svelte-basic	Configuration
@tanstack/query-example-vue-basic	Configuration

[ShacharHarshuv]

Converted to draft as we probably want to add the same thing for injectMutation (and maybe others?)

[riccardoperra]

Shouldn’t properties usually be injected in a class field or inside the query options callback?

Just my opinion, this can lead to unexpected usage from consumers and gives too much freedom 🤔

Angular doesn’t allow to inject inside callback or methods, but as a workaround you can explicitly use runInInjectionContext

[ShacharHarshuv]

I get what you are saying, but in practice, and in many use cases, not being able to do it here just makes DX more difficult.

Here's an example:

cosnt todosQueryOptions = () => {
  const http = inject(HttpClient);

  return queryOptions({ queryKeys: ['todos'], ... });
}

Now I want to invalidate that query (or optimistically update) after creating a new todo, in a type safe way:

onSuccess: async (response) => { queryClient.invalidateQueries(todosQueryOptions()) }

This will fail because onSuccess is not in an injection context, and todosQueryOptions is an injection function.

The way I see it, Angular "injection context" is set in this way because code that doesn't run as initialization of a component could be run from any context and "required" any arbitrary injector. In other words - Angular wouldn't know what injector to use.

However - in this case - since there is a place you are "injecting" the query, it's fair to set it up so that all of the queries work will be (by default) in the same injection context. You can still wrap it in a different injection context with a different injection if you want, but I think that use case will not be very common.

[riccardoperra]

I understood the issue 😄

@Injectable({providedIn: 'root'})
class TodoQuery { 
  readonly http = inject(HttpClient);

  todosQueryOptions() {
    const http = inject(HttpClient);

    return queryOptions({ queryKeys: ['todos'], ... });
  }
} 

then

readonly todoQuery = inject(TodoQuery);

onSuccess: async (response) => { queryClient.invalidateQueries(this.todoQuery.todosQueryOptions()) }

I always used options with a service like above so I've didn't had this error before.

At the moment you have the same issue on react and solid adapters. In solid you can wrap the fn with runWithOwner in order to get the context, in react I think you cannot do it until you create a hook for the query option

function useTodosQueryOptions() { 
   const variable = useContext(...)
   return { ... }
 
}

function App() { 
  const todosQueryOptions = useTodosQueryOptions();

  const query = useQuery(todosQueryOptions());
}

which in Angular the corrispective approach could be doing something like injectTodosQueryOptions

[ShacharHarshuv]

You first example wouldn't work because you are injecting inside the todosQueryOptions method. When you call it in a non-injection context it will throw. I assume you meant that you don't inject that method by use the member instead? I find that approach cumbersome as I would ideally want to encapsulate my dependencies as much as possible, and also separate things like "query options" to different files / symbols and not couple them to the same service I'm using them for.

This becomes more annoying if my query options creator function accepts an argument (like id).

Overall, while workarounds exist, I believe my proposed change improves DX and doesn't have many negative side-effects. cc @arnoud-dv What do you think?

[arnoud-dv]

Matthieu Riegler wrote a good article on inject, why it's not a service locator but can become one when used for injection after initialisation. This PR would enable injection after initialisation and while sometimes convenient it's probably an anti-pattern. So I think we should not enable this functionality and if the developer really wants to do so they can use runInInjectionContext themselves.

I'll also check the existing code in the adapter to see if it only uses inject as intended.

What do you think @ShacharHarshuv @riccardoperra ?

[ShacharHarshuv]

If I understand the article correctly, the main issue with using inject post initialization is that it can cause "easy to miss" exceptions due to missed dependencies. I think this is not relevant though in this case because the query options will always be called at least once immediately after initialization, so any missing dependencies will theoretically be caught.

The only scenario (which I think will not be a common pattern) in which is is problematic is if the inject usage inside the query options callback is dependent on a signal value. But I don't think there is any plausible scenario a consumer will do something like that, especially considering that the queryFn callback is still not run in injection context.

That being said, if you still think it's a bad idea, we can probably close this PR. I would still probably wrap injectQuery in my own code with injectQuery(runInInjectionContext(...)) because I'll likely always want that.

[arnoud-dv]

IMO dependencies should be clearly visible in the code and not have the potential to be different between callback executions. Callback functions of APIs in Angular such as computed or effect don't run in the injection context and I think it's best to align with that.