chore: update effect/platform past API changes #1005
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request includes significant updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
examples/backend-adapters/server/package.json (1)
15-23: Summary: Coordinated update of effect ecosystem dependencies.The changes in this file represent a coordinated update of all effect-related dependencies, which is a good practice for maintaining compatibility. These updates align well with the PR objectives to address dependency issues related to recent API changes in effect/platform.
However, it's important to note that these changes may introduce challenges for users on older versions of effect/platform, as mentioned in the PR objectives. They will need to update their effect dependencies to use the latest version of uploadthing.
Consider adding a note in the project's README or CHANGELOG to inform users about this potential breaking change and provide guidance on how to update their dependencies.
packages/uploadthing/src/sdk/utils.ts (1)
Line range hint
1-223: Overall assessment: Improvements align with PR objectivesThe changes in this file, particularly in the
downloadFilesfunction, align well with the PR objectives of updating dependencies and API usage. The improvements in HTTP request handling and resource management enhance the overall quality of the code.However, it's important to note that these changes might affect users on older versions of effect/platform, as mentioned in the PR objectives. To mitigate potential issues:
- Consider adding a comment in the code to highlight the minimum required version of effect/platform.
- Update the package documentation to clearly communicate the new version requirements.
- If possible, implement a version check to provide a helpful error message for users on incompatible versions.
Would you like assistance in implementing any of these suggestions?
packages/uploadthing/test/adapters.test.ts (1)
867-867: Consider implementing the skipped test to verify customConfigProviderbehaviorThe test
it.effect.skip("still finds the token with a custom config provider", ...)is currently skipped. Implementing this test would verify that using a customConfigProviderdoes not interfere with token retrieval, which is important for users who have their own configurations.Would you like assistance in implementing this test or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
- examples/backend-adapters/client-react/package.json (1 hunks)
- examples/backend-adapters/client-vanilla/package.json (1 hunks)
- examples/backend-adapters/client-vue/package.json (1 hunks)
- examples/backend-adapters/server/package.json (1 hunks)
- examples/backend-adapters/server/src/effect-platform.ts (2 hunks)
- examples/minimal-nuxt/nuxt.config.ts (1 hunks)
- examples/minimal-sveltekit/package.json (1 hunks)
- examples/with-clerk-remix/package.json (1 hunks)
- package.json (2 hunks)
- packages/nuxt/playground/nuxt.config.ts (1 hunks)
- packages/react/package.json (1 hunks)
- packages/shared/package.json (1 hunks)
- packages/svelte/package.json (1 hunks)
- packages/uploadthing/package.json (1 hunks)
- packages/uploadthing/src/internal/handler.ts (9 hunks)
- packages/uploadthing/src/internal/upload.server.ts (3 hunks)
- packages/uploadthing/src/sdk/index.ts (2 hunks)
- packages/uploadthing/src/sdk/utils.ts (3 hunks)
- packages/uploadthing/test/adapters.test.ts (4 hunks)
✅ Files skipped from review due to trivial changes (4)
- examples/backend-adapters/client-react/package.json
- examples/backend-adapters/client-vanilla/package.json
- examples/backend-adapters/client-vue/package.json
- examples/minimal-sveltekit/package.json
🧰 Additional context used
🔇 Additional comments (39)
packages/nuxt/playground/nuxt.config.ts (1)
5-5: LGTM: Telemetry disabled as intended.The addition of
telemetry: falseeffectively disables Nuxt's telemetry feature, which aligns with the PR objectives. This change addresses the issue of user interaction being required during turbo script execution.Implications of this change:
- Improved privacy as no usage data will be sent to Nuxt servers.
- Potential slight performance improvement due to reduced background processes.
- Consistent with development environment best practices.
The code style and placement within the configuration object are appropriate.
examples/minimal-nuxt/nuxt.config.ts (1)
4-4: LGTM: Telemetry disabled as intendedThe addition of
telemetry: falseeffectively disables Nuxt's telemetry feature, which aligns with the PR objective to prevent user interaction during turbo script execution. This change has the following implications:
- It enhances privacy by preventing the collection of usage data.
- It may slightly improve performance by eliminating telemetry-related operations.
- It might indirectly affect future Nuxt improvements that rely on telemetry data.
The implementation is correct and achieves the desired outcome without introducing any apparent issues.
examples/with-clerk-remix/package.json (3)
31-32: Summary: Dependency updates and PR objectivesThe updates to vite (^5.3.1 to ^5.4.8) and vite-tsconfig-paths (^4.3.1 to ^4.3.2) are good practices for maintaining up-to-date dependencies. However, these changes don't appear to be directly related to the effect/platform API changes mentioned in the PR objectives.
To ensure we're not missing any relevant changes, let's check for any effect-related dependencies in the entire project:
#!/bin/bash # Search for any effect-related dependencies in all package.json files rg --type json '"@effect' $(git ls-files | grep package.json)Consider updating the PR description to:
- Include these dependency updates and explain their relevance to the main objective, or
- Clarify if there are other changes in different files that address the effect/platform API changes.
This will help maintain clarity and traceability between the PR description and the actual changes.
31-31: Approved: vite dependency updateThe update of vite from ^5.3.1 to ^5.4.8 is a minor version bump, which is good for keeping the project up-to-date with the latest features and security patches.
However, this change doesn't seem directly related to the effect/platform API changes mentioned in the PR objectives. Let's verify if there are any other changes related to effect:
✅ Verification successful
Verified: effect-related dependencies are updated
The addition of
"@effect/vitest": "0.10.5"inpackage.jsonaligns with the PR objectives related to effect/platform API changes. This update ensures that the project utilizes the latest effect tools and testing capabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any effect-related dependencies in package.json files rg --type json '"@effect' package.jsonLength of output: 72
32-32: Approved: vite-tsconfig-paths dependency updateThe update of vite-tsconfig-paths from ^4.3.1 to ^4.3.2 is a patch version update, which typically includes bug fixes and is safe to apply.
However, like the vite update, this change doesn't seem directly related to the effect/platform API changes mentioned in the PR objectives. It might be helpful to clarify the PR description to include these dependency updates or explain their relevance to the main objective.
To verify if there are any other relevant changes in the PR, let's check for modifications to package.json files:
✅ Verification successful
Approved: Dependency updates
The updates of
vite-tsconfig-pathsfrom^4.3.1to^4.3.2and@effect/vitestfrom0.10.4to0.10.5are both minor version updates, typically including bug fixes and safe to apply.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # List all modified package.json files in the PR git diff --name-only origin/main | grep package.jsonLength of output: 1912
Script:
#!/bin/bash # List all dependency updates in modified package.json files git diff origin/main -- package.json | grep -E '^\+.*"@effect/|^\+.*"vite-tsconfig-paths"'Length of output: 163
examples/backend-adapters/server/package.json (4)
16-16: Approved: @effect/platform-node update is consistent with other changes.The update from 0.58.2 to 0.61.3 for @effect/platform-node is consistent with the update to @effect/platform and aligns with the PR objectives.
To ensure compatibility, please verify that this update doesn't introduce breaking changes:
#!/bin/bash # Description: Check for potential breaking changes in @effect/platform-node # Test: Search for usage of @effect/platform-node in the codebase rg --type typescript -i '@effect/platform-node' -A 5 -B 5
17-17: Approved: @effect/schema update is in line with other effect-related changes.The update from 0.72.2 to 0.74.1 for @effect/schema is consistent with the updates to other effect-related packages and aligns with the PR objectives.
To ensure compatibility, please verify that this update doesn't introduce breaking changes:
#!/bin/bash # Description: Check for potential breaking changes in @effect/schema # Test: Search for usage of @effect/schema in the codebase rg --type typescript -i '@effect/schema' -A 5 -B 5
23-23: Approved: Core effect package update is crucial for alignment.The update from 3.7.2 to 3.8.4 for the main effect package is a key change that aligns with the PR objectives. This update is likely the most critical, as it's probably the core package that the others depend on.
Given the importance of this package, please thoroughly verify that this update doesn't introduce breaking changes:
#!/bin/bash # Description: Check for potential breaking changes in the core effect package # Test: Search for usage of effect in the codebase rg --type typescript -i '\beffect\b' -A 5 -B 5 # Test: Check if there are any migration guides or breaking changes mentioned in the effect package repository echo "Please manually check the effect package repository for any migration guides or breaking changes between versions 3.7.2 and 3.8.4"
15-15: Approved: @effect/platform update aligns with PR objectives.The update from 0.63.2 to 0.66.2 for @effect/platform is in line with the PR's goal to address dependency issues related to recent API changes.
To ensure compatibility, please verify that this update doesn't introduce breaking changes:
examples/backend-adapters/server/src/effect-platform.ts (3)
61-61: LGTM. Ensure thorough testing of HTTP functionality.The change to
Layer.provide(FetchHttpClient.layer)is consistent with the import change and should properly integrate the new HTTP client into the application.To ensure this change doesn't affect the application's HTTP functionality, please verify the following:
- All HTTP requests (GET, POST, etc.) work as expected.
- Error handling for HTTP requests remains intact.
- Any custom configurations or options previously used with
HttpClientare properly migrated toFetchHttpClient.Consider adding or updating tests to cover these scenarios if they don't already exist.
Line range hint
54-54: Address the TODO comment and review for potential updates.There's a TODO comment (
// @ts-expect-error - FIXME!!!) that needs to be addressed. Additionally, while the changes are localized to the HTTP client implementation, it's worth reviewing the unchanged parts of the file to ensure they're fully compatible withFetchHttpClient.Please address the following:
- Investigate and fix the issue causing the
@ts-expect-error.- Review the
uploadthingRoutercreation and usage to ensure it's compatible withFetchHttpClient.- Verify that the CORS middleware and router setup work correctly with the new HTTP client.
Run the following script to check for any
FetchHttpClient-specific methods or properties that might need to be used:#!/bin/bash # Description: Check for FetchHttpClient-specific methods or properties # Test: Search for FetchHttpClient methods in the documentation rg --type markdown 'FetchHttpClient\.(method|property)' ./node_modules/@effect/platform/docsIf any
FetchHttpClient-specific features are found, consider updating the code to take advantage of them.
5-5: LGTM. Verify compatibility with existing code.The change from
HttpClienttoFetchHttpClientaligns with the PR objective of updating dependencies. This should resolve compatibility issues with the latest versions of effect/platform.To ensure this change doesn't introduce any breaking changes, please run the following script:
packages/shared/package.json (3)
Line range hint
3-3: Version update looks good.The package version has been incremented from 7.0.2 to 7.0.3, which is appropriate for the changes made in this update.
Line range hint
1-85: Changes align with PR objectives and address dependency issues.The updates made to this package.json file, including the version increment and the 'effect' dependency update, align well with the PR objectives. These changes address the dependency issues related to effect/platform API changes while maintaining backward compatibility through a patch version update.
39-39: Dependency update approved, but consider documenting potential compatibility issues.The update of the 'effect' dependency from 3.7.2 to 3.8.4 aligns with the PR objectives to address dependency issues related to effect/platform API changes. However, this update might introduce compatibility issues for users on older versions of effect/platform.
To ensure that this update doesn't introduce unexpected breaking changes, please run the following command:
Consider adding a note in the changelog or documentation about potential compatibility issues for users on older versions of effect/platform.
package.json (5)
52-52: Approve @vitest/coverage-v8 update.The update of @vitest/coverage-v8 from ^2.0.5 to ^2.1.2 is a patch version update, which typically includes bug fixes and minor improvements. This change should enhance the testing infrastructure.
60-60: Approve vite-tsconfig-paths update.The update of vite-tsconfig-paths from ^4.3.1 to ^4.3.2 is a patch version update, which typically includes bug fixes and minor improvements. This change should enhance the development environment.
Line range hint
41-61: Summary of dependency updates and final verification.The following dependencies have been updated:
- @effect/vitest: 0.9.2 to 0.10.5
- @vitest/coverage-v8: ^2.0.5 to ^2.1.2
- vite-tsconfig-paths: ^4.3.1 to ^4.3.2
- vitest: ^2.0.5 to ^2.1.2
These updates align with the PR objectives and should improve the project's testing infrastructure and development environment.
To ensure overall compatibility and stability, please run the following verification steps:
- Update dependencies:
pnpm install- Build the project:
pnpm run build- Run all tests:
pnpm test- Check for any deprecation warnings or errors in the console output
If all steps pass without issues, these updates can be confidently merged.
61-61: Approve vitest update and verify compatibility.The update of vitest from ^2.0.5 to ^2.1.2 is a minor version update, which typically includes new features and bug fixes. This change should enhance the testing framework.
Please verify that this update is compatible with the @vitest/coverage-v8 update. Run the following script to check for any potential conflicts or breaking changes:
#!/bin/bash # Description: Check for potential conflicts or breaking changes with vitest update # Test: Search for usage of vitest in the codebase rg --type typescript --type javascript -i '\bvitest\b' # Test: Check if there are any failing tests after the update pnpm test
41-41: Approve @effect/vitest update and verify compatibility.The update of @effect/vitest from 0.9.2 to 0.10.5 aligns with the PR objectives to address dependency issues. This minor version update likely includes new features and bug fixes.
Please verify that this update is compatible with other dependencies in the project. Run the following script to check for any potential conflicts or breaking changes:
packages/svelte/package.json (1)
53-53: LGTM. Verify Vite changelog for potential impacts.The update of Vite from ^5.3.1 to ^5.4.8 aligns with the PR objective of updating dependencies. This minor version update should maintain backwards compatibility.
To ensure there are no breaking changes or significant updates that might affect the project, please review the Vite changelog:
packages/react/package.json (1)
84-84: Approve the @vitest/browser update with verification.The update of @vitest/browser from 2.0.5 to 2.1.2 is a minor version bump, which is generally a good practice for keeping dependencies up-to-date. This change aligns with the PR's objective of updating dependencies.
To ensure this update doesn't introduce any issues, please run the following verification steps:
- Check for any breaking changes or notable updates in the @vitest/browser changelog between versions 2.0.5 and 2.1.2.
- Run the test suite to confirm that all tests still pass with the updated dependency.
- Verify that this update is compatible with other related dependencies in the project.
packages/uploadthing/package.json (1)
153-154: Dependency updates look good, but verify compatibility.The updates to
@effect/platform,@effect/schema, andeffectalign with the PR objectives to address dependency issues related to recent API changes. These are minor version updates, which typically indicate new features or non-breaking changes.To ensure smooth integration:
- Review the changelogs for these packages to understand any potential breaking changes or new features that might affect the codebase.
- Run the following command to check for any compatibility issues or warnings:
This will simulate the installation without actually changing your package.json or node_modules.
Also applies to: 157-157
packages/uploadthing/src/sdk/utils.ts (2)
1-1: LGTM: Import statements updated correctlyThe removal of the
HttpClientResponseimport aligns with the changes in thedownloadFilesfunction. The remaining imports are unchanged and still relevant to the file's functionality.
Line range hint
105-123: LGTM: Improved HTTP request handling and resource managementThe changes in the
downloadFilesfunction enhance its structure and efficiency:
- Introduction of
httpClientvariable improves code readability.- Updated HTTP request execution aligns with the removal of
HttpClientResponseimport.- Using
Effect.flatMapfor error handling provides better control over the response flow.- Addition of
Effect.scopedensures proper resource management.These improvements align with the PR objectives of updating dependencies to recent API changes.
To ensure these changes maintain backward compatibility, please run the following verification script:
This script will help identify any potential compatibility issues with existing code that uses the
downloadFilesfunction.packages/uploadthing/src/internal/upload.server.ts (3)
19-21: Initialization ofhttpClientwithfilterStatusOkis correctThe
httpClientis properly initialized withHttpClient.filterStatusOkto ensure that only successful HTTP responses (status code 200-299) are processed further.
37-37: Addition ofEffect.scopedensures proper resource managementIncluding
Effect.scopedis appropriate here to manage resources effectively during the execution of the effect, ensuring that any acquired resources are properly released.
23-26: Verify the necessity of theRangeheader in the upload requestThe
Range: bytes=0-header is typically used when requesting partial content from a server (for downloads). Including it in an upload request may not be necessary and could potentially cause unintended side effects. Please confirm if this header is required for the upload operation.To verify whether the
Rangeheader is needed in upload requests, you can check if the server expects this header during file uploads.This script searches the entire repository for any TypeScript files that reference the
Rangeheader, which can help determine if it's necessary in this context.✅ Verification successful
Removal of the unnecessary
Rangeheader in upload requestsBased on the codebase analysis, the
Rangeheader is set on the client side but is not processed or utilized on the server side during upload operations. This indicates that theRangeheader is unnecessary and can be safely removed to prevent potential unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search the codebase for server-side handling of the 'Range' header during file uploads. # Test: Find occurrences where the 'Range' header is used in upload handling. # Expected result: Lines of code where the 'Range' header is relevant to uploads. rg --type ts 'Range'Length of output: 1436
Script:
#!/bin/bash # Description: Search the server-side code for handling of the 'Range' header during file uploads. # Using ast-grep to find functions that handle the 'Range' header in upload operations. ast-grep --lang typescript --pattern ' function $_($args) { $_ setHeader("Range", $_) $_ }'Length of output: 99
Script:
#!/bin/bash # Description: Broadly search server-side upload-related TypeScript files for usage of the 'Range' header. # Using ripgrep to find any instances where the 'Range' header is accessed or utilized in upload handling. rg --type ts '(Range)' packages/uploadthing/src/internal/upload.*Length of output: 345
packages/uploadthing/src/sdk/index.ts (6)
2-2: ImportingFetchHttpClientis appropriateThe addition of
FetchHttpClientto the imports from@effect/platformis necessary for the updated HTTP client implementation used in the code below.
81-81: Use ofbodyUnsafeJsonaligns with updated APIThe method
HttpClientRequest.bodyUnsafeJson(body)correctly sets the request body according to the updated API changes. This change ensures the request body is properly handled.
87-87: Executing request withhttpClient.executeis appropriateSwitching to
httpClient.executefor executing the HTTP request aligns with the new implementation, ensuring that the filtered HTTP client is used correctly.
92-93: Proper handling of response withschemaBodyJsonandEffect.scopedUsing
HttpClientResponse.schemaBodyJson(responseSchema)ensures that the response body is parsed and validated against the specified schema, enhancing type safety and robustness. The addition ofEffect.scopedproperly manages resource scoping.
98-98: Updated type signature reflects API changesChanging the type signature in
executeAsynctoEffect.Effect<A, E, HttpClient.HttpClient.Service>reflects the updated types in the@effectlibrary, ensuring compatibility with the new version.
105-112: Correctly providing theFetchHttpClientlayerThe configuration of
FetchHttpClient.layerwith the customfetchimplementation is set up correctly. This ensures that HTTP requests use the providedfetchfunction, which can be important for environments wherefetchneeds to be polyfilled or customized.packages/uploadthing/src/internal/handler.ts (2)
618-626: Verify Metadata for Sensitive Information Before TransmissionThe
metadataobject included in the request body may contain sensitive data or Personally Identifiable Information (PII). Ensure that any sensitive information is sanitized or removed before sending to prevent unintended data exposure.
Line range hint
651-663: Ensure Logging Does Not Expose Sensitive DataWhen logging with
Effect.annotateLogs(chunk), verify that thechunkdoes not contain sensitive information or PII. If it does, consider redacting or omitting sensitive fields from the logs to maintain data privacy and comply with security best practices.packages/uploadthing/test/adapters.test.ts (3)
5-5: Update to import 'FetchHttpClient' looks goodImporting
FetchHttpClientaligns with the recent updates to the@effect/platformAPI.
781-781: Use ofFetchHttpClient.layerin test setup is appropriateReplacing
HttpClient.layerwithFetchHttpClient.layerensures compatibility with the updated API and provides the correct HTTP client layer for the tests.
815-815: Consistent update toFetchHttpClient.layerin test setupMaintaining consistency by updating to
FetchHttpClient.layeracross tests aligns with the API changes and ensures uniformity.
riordanpawley
changed the title
riordanpawley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/uploadthing/src/sdk/index.ts (2)
49-52: LGTM: Improved runtime management with ManagedRuntimeThe addition of the
runtimeproperty and its initialization in the constructor usingmakeRuntimeenhances resource management and error handling. This change aligns well with best practices for managing complex asynchronous operations.Consider adding a brief comment explaining the purpose of the
runtimeproperty for better code documentation.Also applies to: 58-58
69-71: LGTM: Updated HTTP client usage aligns with @effect library patternsThe changes to the
requestUploadThingmethod correctly implement the latest@effectlibrary patterns:
- Using
yield*to accessHttpClient.HttpClientis correct.- Applying
HttpClient.filterStatusOkimproves error handling.- Using
HttpClientRequest.bodyUnsafeJsoninstead ofunsafeJsonBodylikely improves type safety.- Using
httpClient.executefollows the recommended execution pattern.Consider adding a brief comment explaining the purpose of
HttpClient.filterStatusOkfor better code documentation.Also applies to: 75-75, 81-81, 86-87
packages/uploadthing/src/internal/runtime.ts (1)
11-21: Consider adding JSDoc comments formakeRuntimeAdding JSDoc comments to the
makeRuntimefunction would enhance code readability and assist other developers in understanding its purpose, parameters, and usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/uploadthing/src/internal/handler.ts (11 hunks)
- packages/uploadthing/src/internal/runtime.ts (1 hunks)
- packages/uploadthing/src/sdk/index.ts (5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/uploadthing/src/sdk/index.ts (1)
Learnt from: riordanpawley PR: pingdotgg/uploadthing#1005 File: packages/uploadthing/src/sdk/index.ts:75-77 Timestamp: 2024-10-09T22:42:49.319Z Learning: In the 'uploadthing' project's TypeScript code using the '@effect' library, accessing the `HttpClient` service with `(yield* HttpClient.HttpClient)` is correct and should not be flagged as inconsistent.
🔇 Additional comments (8)
packages/uploadthing/src/sdk/index.ts (3)
1-1: LGTM: New imports enhance HTTP handling and runtime managementThe addition of
FetchHttpClient,ManagedRuntime, andmakeRuntimeimports suggests an improvement in HTTP request handling and runtime management. These changes align well with modern TypeScript practices and the@effectlibrary's patterns.Also applies to: 10-10, 27-27
95-101: LGTM: Improved execution and cancellation support in uploadFilesThe changes to the
uploadFilesmethod enhance its functionality:
- Using
this.executeAsyncaligns with the new execution strategy.- Passing the
signaloption toexecuteAsyncprovides better support for operation cancellation, which is particularly useful for file upload operations that may take a while.These improvements make the method more robust and user-friendly.
Line range hint
1-601: Summary: Significant improvements with minor suggestionsThis update to the
UTApiclass brings several improvements:
- Enhanced HTTP client handling and runtime management.
- Better alignment with
@effectlibrary patterns.- Improved async execution and cancellation support.
Consider the following suggestions for further improvement:
- Add brief comments explaining the purpose of new properties and methods.
- Reconsider the runtime disposal strategy in
executeAsyncfor potentially better performance.Overall, these changes significantly enhance the robustness and maintainability of the
UTApiclass.packages/uploadthing/src/internal/runtime.ts (3)
1-3: Imports are correctly definedThe necessary modules are properly imported from
@effect/platformandeffectlibraries.
5-9: Type imports and configurations are appropriateType definitions and configuration imports are correctly specified, ensuring proper typing and configuration setup.
11-21: FunctionmakeRuntimeis correctly implementedThe
makeRuntimefunction appropriately sets up the runtime environment with logging, configuration, and a custom fetch HTTP client. The use ofLayer.provideMerge,Layer.mergeAll, andLayer.setConfigProvideraligns with the expected usage in the@effectlibrary.packages/uploadthing/src/internal/handler.ts (2)
677-680: RefactoredhttpClientInitialization Enhances MaintainabilityDefining
OkHttpClientas a shared utility reduces code duplication and adheres to the DRY principle. This refactoring improves readability and maintainability across the codebase.
95-97:⚠️ Potential issuePotential Issue: Verify Proper Disposal of
managedConsider whether calling
await managed.dispose();before returningresultmight affect the integrity ofresult, especially ifresultrelies on resources managed bymanaged. Verify that all necessary operations are completed before disposing ofmanaged.
| private executeAsync = async <A, E>( | ||
| program: Effect.Effect<A, E, HttpClient.HttpClient.Service>, | ||
| signal?: AbortSignal, | ||
| ) => { | ||
| const layer = Layer.provide( | ||
| Layer.mergeAll( | ||
| withLogFormat, | ||
| withMinimalLogLevel, | ||
| HttpClient.layer, | ||
| Layer.succeed(HttpClient.Fetch, this.fetch as typeof globalThis.fetch), | ||
| ), | ||
| Layer.setConfigProvider(configProvider(this.opts)), | ||
| ); | ||
|
|
||
| return program.pipe( | ||
| Effect.provide(layer), | ||
| const result = await program.pipe( | ||
| Effect.withLogSpan("utapi.#executeAsync"), | ||
| (e) => Effect.runPromise(e, signal ? { signal } : undefined), | ||
| (e) => this.runtime.runPromise(e, signal ? { signal } : undefined), | ||
| ); | ||
|
|
||
| await this.runtime.dispose(); | ||
| return result; |
There was a problem hiding this comment.
Improved async execution, but consider runtime lifecycle
The updates to executeAsync align well with the new managed runtime approach:
- The updated method signature correctly uses
HttpClient.HttpClient.Service. - Using
this.runtime.runPromisefor program execution is appropriate.
However, there's a potential issue:
- Disposing of the runtime (
this.runtime.dispose()) after each execution might be inefficient if theUTApiinstance is intended for multiple operations. This could lead to unnecessary recreation of the runtime for subsequent calls.
Consider implementing a more efficient lifecycle management strategy for the runtime, such as:
- Disposing of the runtime only when the
UTApiinstance is no longer needed. - Implementing a
disposemethod onUTApifor manual cleanup. - Using a singleton runtime instance if appropriate for your use case.
There was a problem hiding this comment.
I agree that UTApi should provide a dispose but I don't want this PR to go into actual uploadthing user facing interface changes. This way keeps the code working the same as before while allowing dispose to be easily moved in the future.
There was a problem hiding this comment.
@riordanpawley, understood. Keeping the current behavior in this PR makes sense to avoid user-facing interface changes. Would you like me to open a GitHub issue to track adding a dispose method to UTApi in the future?
There was a problem hiding this comment.
no, actually thinking on it further - we dont have anything to actually clean up and calling dispose is just best practices for now. lets add the dispose api if we add cleanup logic to our effects in the future
There was a problem hiding this comment.
@riordanpawley, understood. We'll add the dispose API if we add cleanup logic to our effects in the future.
(_/)
( •_•)
(/ ) sigh
✏️ Learnings added
Learnt from: riordanpawley
PR: pingdotgg/uploadthing#1005
File: packages/uploadthing/src/sdk/index.ts:91-101
Timestamp: 2024-10-09T22:55:33.685Z
Learning: Currently, there's no need to add a dispose method to UTApi unless we have cleanup logic in our effects. We'll add it in the future if needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| Effect.flatMap( | ||
| HttpClientResponse.schemaBodyJson(CallbackResultResponse), | ||
| ), |
There was a problem hiding this comment.
Add Error Handling for Response Parsing
When parsing the response with HttpClientResponse.schemaBodyJson(CallbackResultResponse), consider adding error handling to manage cases where the response does not match the expected schema. This will prevent runtime errors and enhance robustness.
There was a problem hiding this comment.
just keeping functionality the same as i'm not sure why the old code had decoding after the logging
There was a problem hiding this comment.
@riordanpawley, understood. Thank you for clarifying.
(｡•́_•̀｡)
| HttpClientResponse.schemaBodyJson(MetadataFetchResponse), | ||
| ), |
There was a problem hiding this comment.
Add Error Handling for Response Parsing
Ensure that when parsing the response with HttpClientResponse.schemaBodyJson(MetadataFetchResponse), appropriate error handling is in place to catch and manage parsing failures or unexpected response formats. This will improve reliability and prevent potential crashes.
There was a problem hiding this comment.
just keeping functionality the same as i'm not sure why the old code had decoding after the logging
There was a problem hiding this comment.
@riordanpawley, understood. Thank you for the clarification.
(_/)
(><)
(")(")
| import { configProvider } from "./config"; | ||
| import { withLogFormat, withMinimalLogLevel } from "./logger"; | ||
|
|
||
| export const makeRuntime = (fetch: FetchEsque, opts?: UTApiOptions) => { |
There was a problem hiding this comment.
This type is misleading as we're also passing in RouteHandlerConfig in handler.ts.
Either an intersection here or split out a common type?
There was a problem hiding this comment.
so a common type describing expected Env variables? alternatives would be passing a full configProvider or expecting a JSON Record
There was a problem hiding this comment.
Json record is probably the most correct, alt a ConfigProvider
There was a problem hiding this comment.
i'll do a follow-up. couldn't push to your fork
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/uploadthing/src/internal/runtime.ts (1)
11-20: LGTM: Well-structured implementation with a minor suggestion.The implementation of
makeRuntimeis logically sound and well-structured. It effectively combines different layers to create a comprehensive runtime environment.Consider adding a type assertion or runtime check for the
fetchparameter before casting it totypeof globalThis.fetch. This could help catch potential incompatibilities earlier. For example:const assertFetch = (f: FetchEsque): asserts f is typeof globalThis.fetch => { if (typeof f !== 'function' || f.length < 2) { throw new Error('Invalid fetch implementation'); } }; assertFetch(fetch); Layer.succeed(FetchHttpClient.Fetch, fetch),This addition would improve type safety without significantly altering the existing logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/uploadthing/src/internal/handler.ts (10 hunks)
- packages/uploadthing/src/internal/runtime.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/internal/handler.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/uploadthing/src/internal/runtime.ts (3)
1-5: LGTM: Imports are appropriate and concise.The imports from
@effect/platform,effect/Layer,effect/ManagedRuntime, and@uploadthing/sharedare well-chosen for the functionality being implemented in this file.
7-8: LGTM: Internal imports are appropriate.The imports from "./config" and "./logger" are relevant to the runtime setup and logging configuration.
1-20: Summary: New runtime creation aligns with PR objectivesThis new file introduces a
makeRuntimefunction that aligns well with the PR objectives of updating dependencies and improving HTTP request handling. The implementation usesFetchHttpClientfrom@effect/platform, which appears to be part of the effort to resolve dependency issues and accommodate recent API changes.The function provides a flexible way to create a runtime environment with custom fetch implementation and configuration, which should help in maintaining compatibility across different versions of effect/platform.
Overall, the implementation is solid, with only minor suggestions for improvement in type safety and error handling. These changes contribute positively to the goals of the PR and should help in resolving the mentioned dependency issues.
|
Thanks for finishing that off for me |
Users that are using effect/platform past a certain version will experience dependency issues if they are trying to use the latest version of uploadthing and the latest version of effect. This PR aims to resolve those issues by updating uploadthing's effect deps past the API changes.Note that this will cause the opposite issue. People on the older effect/platform will need to update their effect deps to upgrade to the latest uploadthing.
(edit) Today pnpm or some other part of my toolchain blessed me by no longer having the above issue
I updated effect and @effect/* deps, but they are behind again - I can update them to latest again if this PR has any interest.
I updated vite, can't remember why but I'm assuming to line up with the effect vitest types.
I turned off nuxt telemetry as it was requiring interaction during a turbo script ¯_(ツ)_/¯
Summary by CodeRabbit
Summary by CodeRabbit
New Features
FetchHttpClientfor improved HTTP handling in the upload functionality.Bug Fixes
Chores
viteacross multiple examples to improve compatibility.uploadthingpackage to7.1.0.