Skip to content
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

feat: change peer dependency version, one type fix and add langgraph in ts docs #1209

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

himanshu-dixit
Copy link
Collaborator

@himanshu-dixit himanshu-dixit commented Jan 18, 2025

Important

Update peer dependency versions, modify type definitions, and add langgraph to TypeScript documentation.

  • Peer Dependencies:
    • Updated versions in package.json and package.dist.json to broader ranges for ai, @ai-sdk/openai, @cloudflare/workers-types, @langchain/core, @langchain/openai, langchain, and openai.
    • Added resolve-package-path as a peer dependency.
  • Type Changes:
    • Modified IntegrationDeleteRes type in integrations.ts to include successful and integrationId fields.
    • Updated ZExecuteActionParams in base_toolset.ts to include optional actionName.
  • Documentation:
    • Added src/frameworks/langgraph.ts to typedoc.json entry points.

This description was created by Ellipsis for 9813885. It will automatically update as commits are pushed.

Copy link

🚀 Code Review Initiated

The review process for this pull request has started. Our system is analyzing the changes for:

  • Code quality, performance, and potential issues
  • Adherence to project guidelines
  • Integration of user-provided learnings

You will receive structured and actionable feedback shortly! ⏳

Copy link

vercel bot commented Jan 18, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 20, 2025 10:48am

Copy link

github-actions bot commented Jan 18, 2025

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-12866347467/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-12866347467/html-report/report.html

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 12ab9dd in 30 seconds

More details
  • Looked at 87 lines of code in 4 files
  • Skipped 1 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/package.json:57
  • Draft comment:
    Avoid using * for versioning in dependencies. Specify a version range for resolve-package-path to ensure compatibility.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_WDGjhbAAg301jl6Q


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

"@langchain/openai": ">0.2.0",
"langchain": ">0.2.0",
"openai": ">4.0",
"resolve-package-path": "*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using * for versioning in peer dependencies. Specify a version range for resolve-package-path to ensure compatibility.

"ai": "^3.2.22",
"langchain": "^0.2.11",
"openai": "^4.50.0"
"ai": ">3.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using > version constraints for peer dependencies could lead to unexpected compatibility issues if major versions introduce breaking changes. Consider:

  1. Using ^ for minor version flexibility while preventing major version jumps
  2. Using more specific ranges like >4.0.0 <5.0.0
  3. Documenting the testing strategy for version compatibility

@@ -35,7 +35,8 @@ export type RawActionData = z.infer<typeof ZRawActionSchema>;
This is the schema for the params object in the ExecuteAction function
*/
export const ZExecuteActionParams = z.object({
action: z.string(),
action: z.string().optional(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making both action and actionName optional could lead to cases where neither is provided. Consider:

  1. Adding a Zod refinement to ensure at least one is provided
  2. Adding JSDoc comments to explain the usage and deprecation
  3. Adding validation in the schema level rather than just in the implementation

Example:

export const ZExecuteActionParams = z.object({
  action: z.string().optional().deprecated(),
  actionName: z.string().optional(),
  // ...
}).refine(
  data => data.action || data.actionName,
  { message: "Either action or actionName must be provided" }
);

@shreysingla11
Copy link
Collaborator

Review Summary

Changes Overview

  1. Package version constraints updated to be more flexible
  2. Type system changes in base_toolset.ts
  3. Added langgraph in documentation
  4. Updated dependencies and lock file

Concerns and Suggestions

  1. Version Management

    • Using > version constraints could lead to compatibility issues
    • Consider using ^ or more specific ranges like >4.0.0 <5.0.0
    • Document testing strategy for version compatibility
  2. Type Safety

    • Both action and actionName being optional needs better validation
    • Add schema-level validation using Zod refinements
    • Improve documentation around the deprecation of action
  3. Documentation

    • Add migration guide for users moving from action to actionName
    • Document version compatibility requirements
    • Update examples to use actionName instead of action
  4. Testing Recommendations

    • Add tests for version compatibility with new peer dependency ranges
    • Add specific tests for action/actionName parameter handling
    • Test integration with latest versions of dependencies

Code Quality: 7/10

  • Good: Clean implementation of parameter handling in executeAction
  • Good: Proper deprecation marking for old parameter
  • Needs Improvement: Schema validation could be stronger
  • Needs Improvement: Version constraints could be more specific

The changes appear well-structured but need additional safeguards around type safety and version management. Consider implementing the suggested improvements before merging.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 9813885 in 22 seconds

More details
  • Looked at 43 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/src/sdk/models/integrations.ts:4
  • Draft comment:
    The import for DeleteRowAPIDTO is removed, but ensure that it is not used elsewhere in the codebase to avoid reference errors.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_Y8ell52pUIeDC9LV


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants