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
fix(angular-query-experimental): run mutation callback in injection context #7360
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit f34b811. 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
Sent with 💌 from NxCloud. |
// check if injection contexts persist in a different task | ||
await new Promise<void>((resolve) => queueMicrotask(() => resolve())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, the test will pass with the previous implementation. That is because the callback of effect
which causes the exception will run in a different task.
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 f34b811:
|
…ontext Previously, the injectMutation callback was run in injection context only if accessed in the same task as the component initialization (i.e. before the `effect` callback run). By running the effect run in injection context, it is ensured that the callback will always run in injection context, and any `inject` calls will not fail. Note there is a separate issue here, and it's that the callback is always run twice - once synchronously when initialization the mutation, and secondly as the first callback of the `effect`. This is something that can potentially be improved. No breaking changes, any code that worked before should still work.
82597e8
to
f34b811
Compare
Resolved conflicts. |
The tests failures don't seem to be related to my change, as they fail in the react adapter. |
Previously, the
injectMutation
callback ran in injection context only if accessed in the same context as the component initialization (i.e. before theeffect
callback run). By wrapping the effect callback in injection context, it is ensured that the callback will always run in injection context, and anyinject
calls will not fail.Note there is a separate issue here, and it's that the callback is always run twice - once synchronously when initialization the mutation (when you create the
MutationObserver
), and secondly as the first callback of theeffect
. This is something that can potentially be improved to prevent redundant code run.Another related issue - I would argue that if any signal dependencies of the mutation changed, you don't necessarily want to immediately run the callback again. You probably want to be "lazy" and run it again only when the mutation object is accessed. With the current implementation, that callback might unnecessarily run when there are a lot of dependency changes, but no mutation is used. (For example, imagine there is an "opened todo item" state, and a delete button on it. The user might switched the opened todo item state frequently without actually using the delete mutation).
No breaking changes, any code that worked before should still work.