-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Add example of lit query adapter #7715
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
Conversation
| @@ -0,0 +1,49 @@ | |||
| import type { QueryClient } from '@tanstack/query-core' | |||
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.
I am not super happy with this solution but I have tried to make it work with lit - Context but I was not able to make it work because of the way it is implemented with the lifecycle.
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.
I can help with the lit context and some controllers. Let me know where/how.
Add example
b7e9b3d to
eee8fd4
Compare
| */ | ||
| hostUpdate() { | ||
| const defaultOption = this.getDefaultOptions() | ||
| this.queryObserver.setOptions(defaultOption) |
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.
This would make the options being set in each lifecycle, which is undesirable. It's preferable to only update the options when they have changed.
| hostConnected() { | ||
| this.unsubscribe = this.queryObserver.subscribe((result) => { | ||
| this.result = result | ||
| this.host.requestUpdate() |
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.
It's preferable to only requestUpdate when the result has changed since a lot of unnecessary updates is triggered otherwise.
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.
I was under the impression that this event listener was only going to get called when the result has changed.
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.
I think you're right. I think all this is handled properly in the queryObserver now.
|
Hello! Thanks a lot for attempting to bridge Lit and Tanstack Query. I tried to make this works locally, just to see. Small, ongoing PR for fixing build, adding a mutation controller, example(s)…: EDIT |
@JulianCataldo Nice to have you onboard! I've made some additions with a context provider also. Gabswim#2 Feel free to try it out. |
Remove unused line Co-authored-by: Gabriel Legault <gablegault1@hotmail.com>
|
Context provider is a huge addition. I'll try this as soon as I can :) Ty! |
Query context provider
|
Thank you for your contributions @JulianCataldo ! I saw your PR, but I’ve been more focused on the one for the context provider by @klasjersevi . Before going any further, I think it would be good to get some feedback from @TkDodo, justinfagnani, just to make sure we're all aligned. One challenge, which I believe has no workaround with Lit’s context, is its asynchronous flow. This issue doesn't seem to exist in other packages, as seen in these implementations: |
Just to explain for anyone who is not familiar with Lit Context: Lit context is an event-based protocol between web components based on the Context Community Protocol by the W3C's Web Components Community Group. Normally this happens synchronously as long as the context provider (parent node) is defined before the context consumer is defined. In some cases this can happen in a different order or if the provider is not yet available for any reason. So the context might be available directly or it might be available at a later point. To overcome some of the challenges with this lit also provides a ContextRoot that can take care of requests until the context provider is available. Many other context api:s in other frameworks are synchronous, which instead throws errors instantly if a context isn't available directly. |
| @property({ type: Number }) | ||
| todoId = 1 | ||
|
|
||
| todoQuery = new QueryController(this, () => ({ |
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.
The ergonomics of this are similar to Lit's Async Tasks. Great work!
|
any progress? |
|
I think we had some good progress with an initial example that should be quite usable. I will soon try to replace some of our custom (very similar) Lit Query controllers with these and see how it works out. It would be great if more people could try these out, so we would know how to proceed moving this forward. |
|
@Gabswim thanks for your all effort on this. |
|
Will this update pushed to the main library? I'd like to know when can I use it because I'd like to use tanstack query on lit-element project now... |
|
sorry I didn’t really look at this because it was a draft PR. I can’t comment much on the lit specific code, but what we would want for an adapter that lives in our monorepo is mainly to have a maintainer who is consistently willing to work on the adapter and who is available for questions on discord. our adapters need to be well maintained because if we make changes to the core that require adapter changes, I need to rely on adapter maintainers to help out. So, if someone wants to step up and take this to the finish line AND is willing to maintain it in a long run, please reach out to me on Discord. Also please note that we have to get the APIs right, because once an adapter is released, it has to stay on the current major version (5) - we can (right now) only bump the major for all adapters. We will likely not make a major version bump for all adapters because the lit adapter needs some tweaks 😅 . That’s also why the I will close this PR for now because the package structure & setup has changed in the meantime and I think it’s easier to just start from scratch, but this doesn’t mean that a lit adapter won’t make it. |
This is just a draft PR to follow up on the discussion I started a while ago here: #6390