-
Notifications
You must be signed in to change notification settings - Fork 790
Revise TypeScript setup instructions and dependencies #1196
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
Updated Node.js and npm version requirements, adjusted project setup instructions, and modified package.json structure.
koverholt
left a comment
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.
Thanks for the PR and improvements! Confirming that the tsc config and compilation step are not needed, and the quickstart is a smoother dev experience after removing them.
I tested all steps with adk-js and things are working for me, except I had to use zod@3 since the PR you linked is not yet merged. Using zod@4 results in an error: Unknown name "def" at 'tools[0].function_declarations[0].parameters'.
So this LGTM once that upstream PR is merged in adk-js. (I'll mark this PR as blocked for now)
Also pinging @joefernandez to review the changes since he worked on the original version of this quickstart.
|
What if we remove the explicit zod dependency? That's what we had in the original get-started, so I think it should still work? (Need to test it.) Then we can do a separate PR for the zod update when that's ready? |
|
Tested! It works. I removed Zod 4. I think it's good to merge if that works for you! |
koverholt
left a comment
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.
Thanks for the updates. Confirming that I tested the latest version and it works as expected without zod, except for an issue with the model string that I left a comment about. Also left some other comments about potential tweaks.
|
And once those comments are addressed, I will pass to @joefernandez for review since this is a heavily visited page / entrypoint. |
|
Thanks for the updates! There is an issue with the CLA check due to commits from a different GitHub account. Can you rebase to switch authorship of those commits, or otherwise fix the failing CLA check? Might have happened due to misconfigured git config user / email. https://github.com/google/adk-docs/pull/1196/checks?check_run_id=61911522036 |
159353e to
b085688
Compare
|
I wasn't smart enough to figure out how to do that, so I'll close this one and open a new PR with the correct email: #1201 |
Updated Node.js and npm version requirements, adjusted project setup instructions, and modified package.json structure.
Might depend on google/adk-js#46