-
Notifications
You must be signed in to change notification settings - Fork 32
TamboStreamProvider (Proof of Concept) #654
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
base: main
Are you sure you want to change the base?
Conversation
…for managing stream states
@armfuldev is attempting to deploy a commit to the tambo ai Team on Vercel. A member of the Team first needs to authorize it. |
Nice, looks like Overall looks good! |
|
||
export interface LoadingProps { | ||
/** The key to identify this loading state */ | ||
key?: string; |
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.
At a very practical level, you can't use key
here
- I'm pretty sure that the value passed for
key
will not actually show up in your props, as it is reserved for react's reconciliation algorithm - Your sample code passes multiple children with the same
key
prop, which is definitely not allowed.
At the very lease this needs a new name, like propKey
or something
https://legacy.reactjs.org/docs/lists-and-keys.html#keys-must-only-be-unique-among-siblings
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.
totally, this was a very quick proof of concept for the team.
i missed that oversight while making the example.
i'm expecting a lot to change according to Tambo requirements
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.
Overall I do like this approach! I'm a little wary of the generic name TamboStreamContext
etc - I feel like this is very specifically for streaming in props and there are other aspects to streaming.
What about TamboStreamPropsProvider
or something? I know it is kind of a mouthful...
i understand your concern, we can tweak this to make more sense. this was mostly to get the idea across to Michael as i showed him a quick example during todays stream. |
@alecf this is good feedback on naming. I like My feedback is mainly around naming. I think we should keep naming to closely match the hook. I'm also wondering if we then provide a flexible
I no longer like this:
On the flip side |
i actually kind of like this:
|
Hey we synced and this is all we decided on:
Then we can merge. Thanks @armfuldev! |
Here's the
TamboStreamProvider
proof of concept you asked for. It provides per-key state management with compound components (Loading
,Empty
,Content
) that work with both streaming and static data, offering a more declarative alternative to manual prop status checking.This was ported from another provider I wrote for Prisma Pulse, but I haven't been able to do proper testing yet. Would love to chat with someone about testing strategies so we can ensure we cover the edge cases you encounter in real usage.
Example usage:
This is just a POC to see if this pattern feels good and could be useful. The idea is to replace stuff like
propStatus["createdAt"]?.isSuccess && <p>{createdAt}</p>
with something more declarative.Would love to get your thoughts on whether this direction feels right! 🤔