-
Notifications
You must be signed in to change notification settings - Fork 620
Break out mastra copilot dependency to subpackage #312
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
Break out mastra copilot dependency to subpackage #312
Conversation
@mme friendly ping! any thoughts on if this is the right approach to break this dependency? Would a separate package be preferred? |
@maxkorp wondering if you can help me out with a review? thoughts on this change? should this function go into a different package? |
Hi @alecf! Let me take a closer look at the uses for this code, we might be able to extract this entirely or something. Otherwise this seems sane. |
Hi @maxkorp any new thoughts/updates here? I am really hoping to integrate mastra into my project here but still blocked by this... |
7cb23d3
to
a177cf4
Compare
(I should add that I'm not as familiar with the tsup stuff - so I'm mostly just following patterns, but I wasn't sure how to get this right for a sub-package) |
@maxkorp friendly ping |
Hey @alecf sorry I dropped the ball on this, I'll test this out today and hopefully get it merged. Thanks so much for this! |
Edit: Nope |
Ok @alecf I was mistaken and we can't remove that. That said, I'm gonna talk to the mastra folks right now to double check, but we should be able to move the copilotkit deps to peerDependencies as well, if you're cool adding that to this PR. Update: I've reached out, I'll let you know when I hear back |
Happy to do that (FWIW unfortunately just moving to peer dependencies isn't enough as its the act of importing the mastra integration that triggers the copilot import, which triggers the build issues) |
Oh, yes I didn't mean to undo the subpackaging. I think having it sub packaged under the mastra integration still makes the most sense (I wish a sub package could auto handle sub-package dependencies in some way but not a necessity), the peer dependencies thing is actually just an accurate change for what it's used for. My understanding is that this is used for automatically wiring up a copilot runtime via your mastra agent, rather than running it separately (and differently from the way we have in the apps/dojo example where we run a mastra agent adjacent to a copilotkit runtime). In that sense it feels like it really should be a peer dep to me. I asked in #309, but what package manager/bundler are you using? The handling of peer deps has changed so much over the last several years and I don't know how consistent things are between npm/yarn/yarn2/yarn3/yarn572/pnpm. Also curious that the dynamic import is getting de-dynamicized, for lack of a better term. |
Mentioned in #309 but we're using nestjs which I think does use webpack under the hood but in this case we're in a server-side context.. also we're just using npm. I'll 🤞 that peerDeps helps here, we've had similar challenges with how that has changed over the years! |
@maxkorp I just noticed the copilot stuff is already in peerDependencies (independent of this PR) |
@maxkorp any progress with mastra folks? |
Sorry, I thought I approved this yesterday! Fired off the tests right now, once those clear we can merge! |
This one approach for #309
This creates a subpackage, @ag-ui/mastra/copilotkit that contains all the copilot dependencies
Alternatively I could move the one function to a separate package entirely @ag-ui/copilotkit or something? This function seems very generic and not directly related to the mastra stuff