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(angular-query-experimental): run query options callbacks in injection context #7362

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShacharHarshuv
Copy link
Contributor

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

…ction context

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

vercel bot commented May 1, 2024

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

Copy link

nx-cloud bot commented May 1, 2024

☁️ 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.

Copy link

codesandbox-ci bot commented May 1, 2024

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
Copy link
Contributor Author

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

@riccardoperra
Copy link
Contributor

riccardoperra commented May 5, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

riccardoperra commented May 5, 2024

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
Copy link
Contributor Author

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?

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

Successfully merging this pull request may close these issues.

None yet

3 participants