-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Adding 'annotations' to component typing for actions #18511
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: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds an optional Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
type: "action"; | ||
methods?: Methods & ThisType<PropThis<ActionPropDefinitions> & Methods>; | ||
props?: ActionPropDefinitions; | ||
annotations?: { |
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.
you should add idempotentHint?
while you're at it, even though we aren't backfilling that value for any components right now
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
types/src/index.ts (1)
387-392
: LGTM! The annotations property addresses the previous feedback.The addition of the
annotations
property with the four boolean hints (includingidempotentHint
as suggested in the previous review) is well-structured. All fields are optional, ensuring no breaking changes.Consider adding JSDoc documentation to clarify the purpose of each hint:
props?: ActionPropDefinitions; + /** + * Optional metadata hints about action behavior characteristics. + * These hints help categorize and optimize action execution. + */ annotations?: { + /** Indicates the action modifies or deletes external resources */ destructiveHint?: boolean; + /** Indicates the action can be safely retried without side effects */ idempotentHint?: boolean; + /** Indicates the action interacts with external systems or APIs */ openWorldHint?: boolean; + /** Indicates the action only reads data without modifications */ readOnlyHint?: boolean; } additionalProps?: (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
types/src/index.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
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 might be doing something wrong, but I get some TypeScript errors when trying to run this package's tests (via npm run test
).
./node_modules/@types/cacheable-request/index.d.ts:26:42 - error TS2709: Cannot use namespace 'ResponseLike' as a type.
cb?: (response: ServerResponse | ResponseLike) => void
Since those don't seem to cause the build to fail, I think they shouldn't be blocking. LGTM!
Edit: Looks like the pnpm publish
actually failed in this check.
To fix this issue, I think you can tell TypeScript to use only type definitions in the current directory, rather than looking for type definitions up the directory tree (e.g. in ../node_modules/@types
). For example, in types/tsconfig.json
, add the following to compilerOptions
:
"typeRoots": ["./node_modules/@types"]
Summary by CodeRabbit