chore: bump effect #1030
chore: bump effect #1030
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request includes updates to several Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
More templates
commit: |
📦 Bundle size comparison
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/shared/src/error.ts (1)
84-91: LGTM with suggestions for improvementThe new validation logic using Effect predicates is more type-safe and explicit. Consider these optional improvements:
- Define a type for the response-like object structure
- Enhance the error message to include more context
Consider this enhancement:
+type ResponseLike = { + status: number; + statusText: string; +}; if ( Predicate.isRecord(opts.cause) && Predicate.isNumber(opts.cause.status) && Predicate.isString(opts.cause.statusText) ) { this.cause = new Error( - `Response ${opts.cause.status} ${opts.cause.statusText}`, + `HTTP Error Response: ${opts.cause.status} ${opts.cause.statusText}`, );packages/uploadthing/src/sdk/utils.ts (1)
84-84: LGTM: Improved type safety with Predicate.isRecordThe replacement of
isObjectwithPredicate.isRecordenhances type safety while maintaining the same functionality. The predicate is used consistently for both URL extraction and property destructuring.Consider adding a type annotation for better code clarity:
- let url = Predicate.isRecord(_url) ? _url.url : _url; + let url: string | URL = Predicate.isRecord(_url) ? _url.url : _url;Also applies to: 104-104
packages/shared/src/utils.ts (2)
Line range hint
171-179: Breaking change:safeParseJSONnow only acceptsResponseEsqueThe implementation looks good, but this is a breaking change as the function no longer accepts string input. Consider documenting this change in JSDoc.
Add JSDoc to document the breaking change:
+/** + * Safely parses JSON from a Response-like object + * @breaking Changed in version X.X.X to only accept ResponseEsque objects + */ export async function safeParseJSON<T>( input: ResponseEsque, ): Promise<T | Error> {
Migration to Effect utilities incomplete - remaining usages found
The verification revealed that
isObjectis still being used in the examples/minimal-expo project while the main packages have migrated to Effect's utilities. This indicates an inconsistency in the migration:
examples/minimal-expo/lib/utils.ts: Contains implementation ofisObjectexamples/minimal-expo/app/api/uploadthing+api.ts: Uses the localisObjectutilityThe main packages show proper migration to Effect utilities:
- Multiple imports of
effect/Predicateandeffect/Arrayacross core packages- No usage of old utilities in the main packages
🔗 Analysis chain
Line range hint
1-1: Verify impact of removed utility functionsThe removal of
isObjectandasArrayfunctions is a breaking change. Let's verify all usages have been properly migrated to Effect's utilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the removed functions echo "Searching for potentially missed usages of removed functions..." rg "isObject|asArray" --type ts # Look for imports from effect/Predicate and effect/Array to verify migration echo "Verifying migration to Effect utilities..." rg "from ['\"](effect/(Predicate|Array))['\"]" --type tsLength of output: 1122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/shared/src/error.ts (2 hunks)
- packages/shared/src/utils.ts (1 hunks)
- packages/uploadthing/src/client.ts (4 hunks)
- packages/uploadthing/src/sdk/index.ts (7 hunks)
- packages/uploadthing/src/sdk/utils.ts (3 hunks)
🔇 Additional comments (16)
packages/shared/src/error.ts (2)
2-2: LGTM: Good choice using Effect predicatesUsing
effect/Predicatefor type validation is a more robust approach than manual object checks.
Line range hint
1-91: Verify location of callbackUrl fixWhile these changes improve error handling, they don't directly address the PR's objective of fixing the
callbackUrlparameter handling. Let's verify where that fix is implemented.packages/uploadthing/src/sdk/utils.ts (2)
3-3: LGTM: Type-safe predicate importThe addition of
Predicatefrom effect/Predicate provides a more robust type-safe alternative to the removedisObjectutility.
Line range hint
1-190: Verify completeness of callbackUrl fixWhile these changes improve type safety, they don't directly address the PR objective of fixing the
callbackUrlhandling issue mentioned in #1027.Let's verify if there are other related changes:
packages/uploadthing/src/client.ts (4)
173-173: LGTM: Type-safe array handling withArr.ensureThe migration from
asArraytoArr.ensureimproves type safety while maintaining the same functionality.
192-192: LGTM: Consistent array handling in resumeUploadThe use of
Arr.ensuremaintains consistency with thepauseUploadimplementation and provides proper type safety.
229-229: LGTM: Type-safe completion handlingThe migration to
Arr.ensurecompletes the consistent pattern across all upload control functions while maintaining proper type safety and Promise handling.
1-1: Verify callbackUrl changes in other filesWhile the array handling improvements in this file look good, the PR objectives mention fixing a
callbackUrlissue which isn't addressed here.Let's verify where the
callbackUrlchanges are implemented:packages/uploadthing/src/sdk/index.ts (8)
20-20: LGTM: Import cleanupRemoval of the
asArrayimport aligns with the migration toArr.ensurefromeffect/Array.
130-130: LGTM: Consistent array handlingThe use of
Arr.ensuremaintains type safety while aligning with the Effect-based architecture.
176-176: LGTM: Robust array handlingThe implementation correctly handles both single and array inputs while maintaining error handling.
242-243: LGTM: Type-safe array handlingThe implementation correctly handles array conversion for both
fileKeysandcustomIdscases.
283-284: LGTM: Consistent implementationThe array handling mirrors the pattern used in
deleteFiles, maintaining consistency across the codebase.
344-344: LGTM: Clean array conversionThe implementation maintains type safety while simplifying array handling.
Line range hint
435-439: LGTM: Efficient array processingThe implementation efficiently combines array conversion with mapping while maintaining type safety.
Line range hint
1-452: Verify alignment with PR objectivesThe changes in this file focus on replacing
asArraywithArr.ensurefor array handling, but the PR objectives mention addressing acallbackUrlparameter issue. Please verify if additional changes are needed to address thecallbackUrlfunctionality described in issue #1027.
Closes #1027
Summary by CodeRabbit
New Features
Bug Fixes
Improvements