feat: add custom fetch override #1099
Conversation
🦋 Changeset detectedLatest commit: 6890016 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces a comprehensive enhancement to the UploadThing library, focusing on adding a custom fetch implementation across multiple packages. The changes allow developers to override the default Changes
Sequence DiagramsequenceDiagram
participant Developer
participant UploadThing
participant CustomFetch
participant Server
Developer->>UploadThing: Initialize with custom fetch
UploadThing->>CustomFetch: Use provided fetch function
CustomFetch->>Server: Make network request
Server-->>CustomFetch: Return response
CustomFetch-->>UploadThing: Provide response
UploadThing-->>Developer: Complete upload
Possibly related PRs
Suggested labels
Suggested reviewersmarkflorkowski 🪧 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
@uploadthing/expo
@uploadthing/nuxt
@uploadthing/react
@uploadthing/shared
@uploadthing/solid
@uploadthing/svelte
uploadthing
@uploadthing/vue
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
🧹 Nitpick comments (2)
packages/uploadthing/src/internal/upload.browser.ts (1)
136-146: Ensure concurrency usage meets your scaling needsThe new concurrency for the Micro.forEach is set to 6. This might be fine in most scenarios, but confirm it matches the intended upload concurrency constraints. If you foresee higher concurrency demands, consider parameterizing this or making it adjustable.
packages/uploadthing/test/client.test.ts (1)
240-245: Custom fetch function: Maintain security best practicesIf sending cookies, tokens, or cross-origin requests, ensure you handle CORS, credentials, and other security aspects. This is more of a usage advice than a code concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/uploadthing/src/client.ts(6 hunks)packages/uploadthing/src/internal/upload.browser.ts(3 hunks)packages/uploadthing/src/types.ts(1 hunks)packages/uploadthing/test/client.test.ts(4 hunks)
🔇 Additional comments (14)
packages/uploadthing/src/internal/upload.browser.ts (2)
4-5: LGTM: Additional import of FetchContext is consistent with usage
Importing both FetchContext and FetchError from "@uploadthing/shared" is a straightforward extension. No issues here.
122-123: Great enhancement of return type to include FetchContext
Extending Micro.Micro to have a third type parameter for FetchContext provides better type safety and clarity regarding the fetch context.
packages/uploadthing/src/types.ts (1)
210-228: New 'fetch' property for GenerateUploaderOptions: Great addition!
Introducing an optional custom fetch function broadens the library's extensibility. The detailed example comments (lines 211-226) are helpful for users.
packages/uploadthing/src/client.ts (4)
4-4: Importing FetchEsque type clarifies custom fetch usage
Using the FetchEsque type from "@uploadthing/shared" ensures strong typing for your custom fetch approach.
145-145: Consistent usage of provideService ensures fetch context is set
Using Micro.provideService for the custom fetch aligns fetch context across different calls. Good job keeping it consistent.
Line range hint 258-268: Typed upload: Great approach for robust error handling
The typedUploadFiles function uses provideService with the new or default fetch function. The error handling with Micro.runPromiseExit is well-structured.
Line range hint 115-127: Custom fetch assignment looks good
The fallback to window.fetch is logical, and the Micro pipeline usage is consistent with typical effect pattern. Just ensure the environment always has a fetch available (e.g., Node/SSR setups might need a polyfill).
✅ Verification successful
Let me verify the fetch availability and polyfill concerns by examining the codebase.
This will help us understand:
- How fetch is handled across different environments
- If there are any polyfill dependencies
- Any environment-specific checks related to fetch availability
Fetch implementation is well-handled across environments
The codebase has robust fetch handling:
- Custom fetch can be provided via
initOpts.fetch - Falls back to
window.fetchin browser environments - Uses
globalThis.fetchin the SDK for isomorphic support - Has proper type definitions (
FetchEsque) and interfaces for fetch implementations - Includes tests verifying custom fetch functionality
The implementation is safe and follows best practices for cross-environment compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check for fetch polyfill or related imports/configurations
rg -g '*.{ts,js,tsx,jsx}' -i "fetch|polyfill" packages/uploadthing/
# Look for environment checks or isomorphic fetch handling
ast-grep --pattern 'typeof window !== "undefined"'
# Check package.json for fetch-related dependencies
cat packages/uploadthing/package.json | jq '.dependencies, .peerDependencies'
Length of output: 9098
packages/uploadthing/test/client.test.ts (7)
9-12: Type import for GenerateUploaderOptions
Importing GenerateUploaderOptions clarifies the client configuration within the test suite. No issues noted.
26-28: Optional clientOpts in setupUTServer improves test flexibility
Allowing partial GenerateUploaderOptions fosters custom testing scenarios such as different fetch implementations or URLs.
237-239: Custom fetch test scenario is well-structured
The vi.fn() usage ensures your test verifies that fetch was indeed invoked. Good approach.
247-249: uploadFiles invocation triggers your custom fetch as expected
This ensures the new fetch method is used in real uploads. Test coverage here is valuable.
250-257: Test robustly verifies the presence of POST request
The test ensures the request method is POST with a FormData body, matching your typical usage pattern. LGTM.
258-259: Cleanup at the end is a best practice
Closing the server after the test run helps prevent resource leaks and port binding conflicts.
98-98: Spread operator for clientOpts merges default and custom config
Merging clientOpts with the default config is a simple and effective approach. Confirm that merging occurs in the correct order to avoid overwriting critical fields.
✅ Verification successful
Let me gather more information about how clientOpts is being used in the codebase.
Let me check the implementation of genUploader to understand the default configuration and merging order.
Let me check the implementation of uploadFilesInternal to see how the configuration is merged.
Based on the code analysis, I can now provide the final response:
Spread operator usage for clientOpts is correctly implemented
The spread operator in ...clientOpts is used appropriately in the test setup. The configuration merging occurs in the correct order where clientOpts is spread after the default values (url and package), ensuring custom configurations can override defaults without losing critical fields. The implementation in genUploader and uploadFilesInternal properly handles this merged configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the file containing the test and show surrounding context
rg -C 10 "clientOpts" "packages/uploadthing/test/client.test.ts"
# Look for the default configuration that might be merged with clientOpts
ast-grep --pattern 'const $_ = {$$$}' "packages/uploadthing/test/client.test.ts"
Length of output: 4190
Script:
#!/bin/bash
# Find the genUploader implementation
ast-grep --pattern 'const genUploader = $_' || ast-grep --pattern 'function genUploader($_) {$$$}'
# Also search for any type definitions that might show the default values
rg -C 5 "type GenerateUploaderOptions"
Length of output: 15642
Script:
#!/bin/bash
# Find uploadFilesInternal implementation and its usage of configuration
ast-grep --pattern 'const uploadFilesInternal = $_' || ast-grep --pattern 'function uploadFilesInternal($_) {$$$}'
# Also look for any default configuration objects
rg -C 5 "uploadFilesInternal" "packages/uploadthing/src"
Length of output: 8416
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react/src/components/dropzone.tsx (1)
149-149: Ensure environment compatibility for fallback fetch.Using $props.fetch or defaulting to globalThis.fetch is a clean approach that respects custom fetch implementations. However, if this code runs in older or specialized environments where globalThis.fetch is not defined, you may need a fallback or polyfill.
packages/react/src/components/button.tsx (1)
114-114: Validate custom fetch usage and fallback handling.Similar to the dropzone component, this line properly defaults to globalThis.fetch if $props.fetch is undefined. Confirm that the environment (e.g., SSR) supports globalThis.fetch or include a suitable polyfill if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/react/src/components/button.tsx(1 hunks)packages/react/src/components/dropzone.tsx(1 hunks)packages/react/src/components/index.tsx(3 hunks)packages/react/src/types.ts(3 hunks)packages/react/src/useUploadThing.ts(5 hunks)packages/react/src/utils/useFetch.ts(2 hunks)packages/uploadthing/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/types.ts
🔇 Additional comments (19)
packages/react/src/components/index.tsx (4)
31-31: Confirm default fetch fallback.
This line correctly falls back to the global fetch if none is provided, ensuring backward compatibility.
38-44: Pass fetch prop to UploadButton.
Forwarding the custom fetch prop to the UploadButton aligns with the new configurability requirement.
64-70: Forward fetch prop to UploadDropzone.
This implementation ensures a consistent way to use the custom fetch within the dropzone component.
90-92: Include fetch prop in generateUploader.
Propagating the custom fetch to Uploader maintains consistency with the other components.
packages/react/src/utils/useFetch.ts (2)
4-4: Ensure type consistency for FetchEsque import.
Importing FetchEsque from "@uploadthing/shared" enables a consistent type definition across the codebase.
20-24: Parameterizing fetch function.
Switching to a passed-in fetch function significantly increases flexibility, allowing usage of custom fetch implementations in different environments.
packages/react/src/types.ts (3)
5-5: Use of FetchEsque type.
Importing FetchEsque ensures consistent references to the custom fetch definition across the application.
30-48: Optional fetch property with fallback.
Defining an optional fetch property broadens configurability while retaining the global fetch fallback. This is a clean approach.
145-163: Extended UploadthingComponentProps with fetch.
Adding the fetch property strengthens the typed contract for components, letting users explicitly provide a custom fetch if needed.
packages/react/src/useUploadThing.ts (10)
6-6: Accurate import of FetchEsque.
Ensures the module uses the standardized type for a custom fetch function.
34-34: Augment globalThis.
Enhancing globalThis typing to include FetchEsque is valid for broader usage in this module.
38-38: Parameter for useRouteConfig.
Passing the fetch function to this helper ensures fetched route configurations always rely on the chosen fetch implementation.
44-44: Inject fetch into useFetch invocation.
This approach replaces any default fetch usage in useFetch, aligning with the new custom fetch architecture.
62-62: Updated __useUploadThingInternal signature.
Requiring fetch in the internal hook is key for consistent usage across all uploads.
68-68: Assign fetch to genUploader.
Loading the custom fetch config places the user-defined fetch at the heart of the upload logic.
143-143: Leverage useRouteConfig with a custom fetch.
Fetching route data with a user-defined fetch ensures every piece of data retrieval uses consistent logic.
161-161: Conditional assignment for fetch.
This fallback mechanism is aligned with the overall design, ensuring no breakage if a custom fetch is not supplied.
167-167: Initialize genUploader with a custom fetch.
Maintaining the fetch prop at this stage allows the entire upload pipeline to remain user-configurable.
174-174: Integrate fetch into useUploadThing.
Methodically passing fetch onward ensures the custom fetch consistently drives network operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/expo/src/index.ts (1)
24-42: Add usage guidance or references wherever possible.The in-code example for customizing fetch is helpful. Consider also referencing any docs or type definitions clarifying “FetchEsque” usage for clarity, so developers know potential differences from the standard fetch.
packages/expo/src/document-picker.ts (1)
40-40: Consider making fetch optional if default is expected.Currently, “fetch” is marked as a required property. If you want alignment with the fallback logic from “index.ts”, you could consider making it optional here as well, unless you intend for these specialized hooks to always pass a fetch.
-}(initOpts: { - url: URL; - fetch: FetchEsque; -}) => { +}(initOpts: { + url: URL; + fetch?: FetchEsque; +}) => { const useDocumentUploader ...packages/expo/src/image-picker.ts (2)
7-11: Avoid potential naming collisions for "FetchEsque".The naming is intuitive, but if the project evolves, consider a clearer name (e.g. “CustomFetchType”) to avoid confusion with potential other custom fetch type definitions.
32-32: Consistent optional property usage.As with “document-picker.ts”, it may be beneficial to allow “fetch” to be optional here as well.
export const GENERATE_useImageUploader = < TRouter extends FileRouter, >(initOpts: { url: URL; - fetch: FetchEsque; + fetch?: FetchEsque; }) => { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/expo/src/document-picker.ts(3 hunks)packages/expo/src/image-picker.ts(3 hunks)packages/expo/src/index.ts(3 hunks)packages/react/src/useUploadThing.ts(5 hunks)packages/uploadthing/test/client.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/uploadthing/test/client.test.ts
- packages/react/src/useUploadThing.ts
🔇 Additional comments (6)
packages/expo/src/index.ts (3)
4-4: Ensure correct usage of imported types.
The newly introduced "FetchEsque" type clarifies the signature for custom fetch implementations. This looks good and follows TS best practices.
78-80: Collocate usage of fetch across other helper calls.
Multiple helpers (vanillaHelpers, useImageUploader, and useDocumentUploader) are passed the same “opts”. This is consistent and DRY, which is good. Confirm these helpers align with the new optional fetch logic.
71-76: Validate fallback logic.
When “initOpts.fetch” is undefined, this defaults to “globalThis.fetch”. Ensure that in React Native environments or other contexts, “globalThis.fetch” is reliably available or polyfilled. In some RN setups, fetch may be polyfilled differently.
✅ Verification successful
Let me gather information about how fetch is handled in the codebase and any potential polyfill configurations.
Let me search for React Native specific configurations and fetch polyfills in the codebase.
Based on the search results, I can see that:
-
The code in
packages/expo/src/index.tsalready handles fetch implementation gracefully by:- Allowing custom fetch implementation through
initOpts.fetch - Falling back to
globalThis.fetchwhen no custom implementation is provided - Providing examples of how to customize fetch behavior for specific use cases
- Allowing custom fetch implementation through
-
The codebase includes React Native examples (minimal-expo) that use this fetch implementation without any issues.
-
The implementation is used consistently across image and document pickers in the Expo package.
Given this evidence, I can generate the final response:
The fetch fallback implementation is properly handled and secure.
The current implementation follows best practices by:
- Allowing dependency injection of custom fetch implementations
- Using the standard
globalThis.fetchas a fallback, which is reliably available in modern React Native versions - Providing clear documentation and examples for customizing fetch behavior when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for fetch-related configurations and polyfills
rg -l "fetch" --type ts --type js
# Look for React Native specific configurations
rg -l "react-native" --type json
# Check package.json for dependencies that might handle fetch polyfills
fd "package.json" --type f --exec cat {}
# Look for any fetch-related documentation
fd "README.md" --type f --exec grep -l "fetch" {} \;
Length of output: 55811
Script:
#!/bin/bash
# Look for React Native specific fetch configurations
rg -A 5 "fetch" packages/expo/src/
rg -A 5 "fetch" examples/minimal-expo/
# Check for any fetch polyfill configurations
rg -l "polyfill.*fetch|fetch.*polyfill" --type ts --type js
# Look for any React Native specific setup files
fd "setup|config|polyfill" --type ts --type js --exec cat {}
Length of output: 5563
packages/expo/src/document-picker.ts (2)
7-11: Confirm side effects of "FetchEsque" import.
The introduction of “FetchEsque” is consistent with the approach in “index.ts”. No direct issues observed.
49-49: Propagate custom fetch consistently.
The call to “__useUploadThingInternal” correctly passes “initOpts.fetch”. Ensure the underlying function handles undefined fetch gracefully if it ever becomes optional in the future.
packages/expo/src/image-picker.ts (1)
41-41: Verify usage fallback.
Similar to “document-picker.ts”, confirm that “__useUploadThingInternal” is prepared for a missing “fetch” property. If not, ensure a fallback is handled.
✅ Verification successful
Let me gather information about the usage of fetch and __useUploadThingInternal to verify the fallback handling.
Let me fix the search pattern and try again.
Let me search for the implementation of __useUploadThingInternal to verify how it handles the fetch property.
Let me search for the implementation in the useUploadThing file since it's imported from there.
After reviewing the implementation, I can now provide a final response:
The fetch property is properly handled with fallbacks.
Looking at the implementation in useUploadThing.ts, the fetch property is handled with a fallback mechanism in two places:
- In
generateReactHelpers, it usesinitOpts?.fetch ?? globalThis.fetch - The
__useUploadThingInternalfunction properly receives and utilizes the fetch implementation passed down through the initialization options
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for __useUploadThingInternal implementation and its usage of fetch
ast-grep --pattern 'function __useUploadThingInternal($_) {
$$$
}'
# Also search for any direct references to fetch property
rg "fetch\:" -A 5 -B 5
Length of output: 174
Script:
#!/bin/bash
# Search for __useUploadThingInternal implementation
ast-grep --pattern 'export function __useUploadThingInternal($_) {
$$$
}'
# Search for fetch property usage without escape character
rg "fetch:" -A 5 -B 5
# Also search for the file content to understand the context
cat packages/expo/src/image-picker.ts
Length of output: 27029
Script:
#!/bin/bash
# Search for __useUploadThingInternal implementation
ast-grep --pattern 'export function __useUploadThingInternal($_) {
$$$
}'
# Also search for the file content directly
cat packages/react/src/native.ts
Length of output: 371
Script:
#!/bin/bash
# Search for the implementation in useUploadThing.ts
cat packages/react/src/useUploadThing.ts
# Also search for any fetch-related error handling
rg "fetch.*error" -A 5 -B 5
Length of output: 7508
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (18)
packages/uploadthing/test/adapters.test.ts (16)
38-41: Server Lifecycle Best Practice
Usingserver.listenandserver.closeinbeforeAllandafterAllis good practice. However, ensure that any additional custom mocks are cleared or reset inafterEachto avoid potential contamination between tests.
60-60: Clarify Test Description
The test title "returns router config on GET requests" is concise but consider making it more descriptive, like "returns correct router config JSON on GET requests".
82-82: Ensure Large File Upload Cases
The test checks for a single small file. If we want robust coverage, add or update a test to see how the middleware handles larger or multiple file uploads.
171-171: Test Title Uniformity
For consistency with the h3 adapter tests, you might rename "returns router config on GET requests" to something like "returns correct router config JSON on GET requests for server adapter."
191-191: Consider Testing Different Content Types
Currently, the test only checks Content-Type:application/json. If your app supports other content types (e.g., multipart/form-data), consider adding a test scenario for that as well.
249-249: Validation of Optional Parameters
The test "accepts object with request in" is a great addition. Consider adding coverage for optional or missing properties (like missingrequest) to ensure graceful handling.
326-326: Add Consistency for .GET vs .POST
TheeventHandler.GET()usage is consistent with the NextRequest style, but consider verifying.HEADor.OPTIONStoo if your router might handle them.
348-348: Check Request Edge Cases
The test verifies a single typical.POST(). Add edge-case testing (e.g., empty file array, invalid JSON) to confirm robust error handling.
461-461: Confirm Headers in NextApiRequest
Since Next.js might have custom headers in certain hosting environments (Vercel, for instance), consider adding a test to confirm these are handled if they are critical to your logic.
485-485: Validate Non-JSON Body
While the test covers JSON request data, it might be beneficial to ensure non-JSON or malformed JSON payloads produce correct responses (e.g., 4xx).
584-584: Enhance GET Endpoint Testing
Like the other adapters, ensure we test any custom routes or user-defined middlewares in addition to the standard GET.
602-602: Add Additional Assertions
We see a single "should proceed to generate a signed URL" block. Consider verifying the shape and content of that URL beyond a regex, to confirm domain or path correctness.
662-662: Middleware Confirmation
The test "works with some standard built-in middlewares" is a good start. If you support advanced features like compression or caching middlewares, consider adding coverage for those.
682-682: Consider Using a Single Body-Parsing Library
We see a separate test scenario forbody-parser. Confirm consistency in your recommended approach to parsing request bodies across different frameworks.
738-738: Confirm Additional Fastify Scenarios
The test verifies GET requests. Also verify how your route behaves when receiving unexpected HTTP methods (like PUT or PATCH) if relevant to your adapters.
756-756: Potential Race Conditions
After verifying the final request toINGEST_URLis queued, confirm that parallel or concurrent uploads are properly handled, especially for large payloads.package.json (1)
50-53: Consolidate Test Dependencies
You now have bothmswand@vitest/browserinstalled. Check if the usage is consistent or if some older test dependencies can be removed from child packages to reduce duplication..github/workflows/ci.yaml (1)
34-37: Consider caching Playwright browsers.The Playwright installation step is correctly positioned and uses the minimal browser set needed. However, to optimize CI performance, consider caching the Playwright browser binaries.
Add browser caching by applying this diff:
- name: Build run: pnpm turbo build --filter "./packages/*" + - name: Cache Playwright browsers + uses: actions/cache@v3 + id: playwright-cache + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-chromium + - name: Install Playwright + if: steps.playwright-cache.outputs.cache-hit != 'true' run: pnpm exec playwright install chromium
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/ci.yaml(1 hunks)examples/backend-adapters/package.json(1 hunks)package.json(1 hunks)packages/react/package.json(1 hunks)packages/react/test/client-generator.test.ts(1 hunks)packages/react/test/upload-button.browser.test.tsx(12 hunks)packages/react/test/upload-dropzone.browser.test.tsx(5 hunks)packages/react/vitest.config.ts(0 hunks)packages/uploadthing/test/__test-helpers.ts(3 hunks)packages/uploadthing/test/adapters.test.ts(16 hunks)packages/uploadthing/test/client.browser.test.ts(10 hunks)packages/uploadthing/test/request-handler.test.ts(18 hunks)packages/uploadthing/test/sdk.live.test.ts(1 hunks)packages/uploadthing/test/sdk.test.ts(20 hunks)packages/uploadthing/test/upload-builder.test.ts(0 hunks)packages/uploadthing/vitest.config.ts(0 hunks)patches/@[email protected](0 hunks)patches/[email protected](0 hunks)vitest.workspace.ts(1 hunks)
💤 Files with no reviewable changes (5)
- packages/uploadthing/vitest.config.ts
- packages/react/vitest.config.ts
- patches/[email protected]
- packages/uploadthing/test/upload-builder.test.ts
- patches/@[email protected]
🧰 Additional context used
🪛 GitHub Check: build
packages/uploadthing/test/sdk.live.test.ts
[failure] 65-65: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should upload a file
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89I5iY5NnbUlVkevf5bcZGNoLy26pw7XI3ODmjR", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919930323, "name": "foo.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89I5iY5NnbUlVkevf5bcZGNoLy26pw7XI3ODmjR",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:65:22
[failure] 101-101: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should upload a private file
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89IIPJue9MGkZXF0NudKOg2JioQU9fSrqEHmAIt", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919932843, "name": "foo.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89IIPJue9MGkZXF0NudKOg2JioQU9fSrqEHmAIt",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:101:22
[failure] 132-132: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should upload a file from a url
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89INBGOdPauf9W1zAX4dnGIJywloj3qi780TYgP", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": Any<Number>, "name": "favicon.ico", "size": Any<Number>, "type": "image/vnd.microsoft.icon",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89INBGOdPauf9W1zAX4dnGIJywloj3qi780TYgP",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:132:22
[failure] 157-157: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should rename a file with fileKey
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89I2ybL6RiGHj7eSFgd68wM9or4mx2JYyqIBzTU", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": Any<Number>, "name": "bar.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89I2ybL6RiGHj7eSFgd68wM9or4mx2JYyqIBzTU",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:157:22
[failure] 201-201: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should rename a file with customId
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { data: { …(9) }, error: null }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89IaOBjqaDBFgnWez4rqkHEVGPhb0sCfNOyQUjZ", "customId": "ba44ad17-0e0f-466b-9bb7-39d58f74bed3", "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919938995, "name": "bar.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89IaOBjqaDBFgnWez4rqkHEVGPhb0sCfNOyQUjZ",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:201:22
[failure] 242-242: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should update ACL
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89I364mLTEXFuHUGrj5WXe3Cn0bx6fPNozmBAZd", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919941005, "name": "foo.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89I364mLTEXFuHUGrj5WXe3Cn0bx6fPNozmBAZd",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:242:22
[failure] 288-288: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should have correct usage info
AssertionError: expected GetUsageInfoResponse{ …(4) } to deeply equal { totalBytes: +0, …(3) }
- Expected
- Received
- Object {
- "appTotalBytes": 0,
- "filesUploaded": -1,
- GetUsageInfoResponse {
- "appTotalBytes": 15418,
- "filesUploaded": 5,
"limitBytes": 2147483648,
- "totalBytes": 0,
- "totalBytes": 15418,
}
❯ packages/uploadthing/test/sdk.live.test.ts:288:25
🪛 GitHub Actions: CI
packages/uploadthing/test/sdk.live.test.ts
[error] 65-65: Test failure: 'should upload a file' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 101-101: Test failure: 'should upload a private file' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 132-132: Test failure: 'should upload a file from a url' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 157-157: Test failure: 'should rename a file with fileKey' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 201-201: Test failure: 'should rename a file with customId' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 242-242: Test failure: 'should update ACL' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 288-288: Test failure: 'should have correct usage info' - Mismatched values for appTotalBytes (expected: 0, got: 15418) and filesUploaded (expected: -1, got: 5)
🔇 Additional comments (98)
packages/react/test/upload-button.browser.test.tsx (18)
1-5: Imports look good
All imports have been successfully changed to browser-specific testing context, userEvent, and msw-related modules. This aligns well with the new browser-based testing approach.
37-37: Mock worker setup is properly configured
Switching to the browsersetupWorkerfrommsw/browseris appropriate for client-side tests. This looks good.
55-56: Browser worker lifecycle
Good use ofbeforeAllandafterAllto manage the MSW worker lifecycle. Ensures clean test runs.
98-100: Wait state check
Usingvi.waitForensures the label is fully loaded before testing. No concerns here.
105-105: Visibility check on label
Usingawait expect.element(page.getByText("Image (4MB)"))helps confirm that MSW is returning the correct route config. Looks good.
120-122: Callback endpoint assertion
Verifying endpoint resolution through a callback is tested thoroughly. Good approach to ensure dynamic endpoints work.
127-127: Visibility check on label
Again, using the page-based assertion aligns with the new test environment. No issues found.
140-142: Plural label
Proper check to ensure the label reflects multiple file uploads. Implementation is correct.
150-150: Global route config test
Nice strategy to confirm that the global__UPLOADTHINGapproach skips fetching. Good coverage.
159-174: File upload and route request
Verifies the file gets uploaded and triggers the correct mocks:
- The file size is set to 18, likely reflecting changed file encoding.
- Ensures that the
utPostmock is called with the accurate body.
Everything looks correct and thorough.
189-191: Ready state check
Ensuring the component is in a ready state before next actions. Straightforward.
193-195: Manual mode file selection
The test verifies the label text is correct after selection. Good clarity for the manual upload mode.
198-199: Explicit upload trigger
Tests that the extra user click triggers the upload in manual mode. Correct coverage of user flow.
214-214: Skipped test
The disabled test is intentionally skipped. Keeping it around for future coverage is reasonable.
230-235: onBeforeUploadBegin
Confirms that altering the file list at the last moment works, substituting "foo.txt" with "bar.txt". Nicely tested.
252-258: onUploadBegin with multiple files
Ensures this hook fires for each file. This properly validates multi-file flows.
276-280: Theming test
Good test verifying custom style overrides appear in the UI.
287-295: Custom content
Ensures that user-specified content is displayed. Straightforward and correct.packages/uploadthing/test/client.browser.test.ts (13)
1-3: MSW and browser test setup
Switch tosetupWorkerfrommsw/browserfor client tests is consistent with the rest of the PR.
31-33: Worker lifecycle
Worker started and stopped inbeforeAll/afterAll. Clean test environment setup.
35-37: setupUTServer param
The optionalclientOptsparam for customizing or overriding fetch is a great addition for more flexible test scenarios.
96-98: Dynamic ephemeral port
Using a random port (port = Math.floor...) for local dev/test conflicts is a good practice.
102-103: Merging client options
SpreadingclientOptsinto thegenUploadercall is an elegant way to override fetch or other config settings.
108-110: Mock reset on close
Reverting to baseline handlers on close is a safe approach, ensuring subsequent tests aren’t affected.
114-127: awaitServerData coverage
Properly tests config differences whereawaitServerDatais bothtrueandfalse. The usage ofdoNotExecuteis presumably to skip actual calls in test. Looks consistent.
Line range hint
171-199: Custom static headers
Verifying that user-supplied headers are included in the request is critical. The approach is correct:
- Using
Object.fromEntrieson the request headers ensures easy introspection.
Line range hint
208-236: Custom async headers
Same concept as above but async. Confirmed logic to wait on the resolved headers. Great coverage.
245-261: Custom fetch override
Successfully demonstrates that the provided fetch function is called. This is the main feature of the PR. Great test coverage.
Line range hint
291-300: Exceeding file size
Validates user gets the proper error when file is too large. Good negative test scenario.
Line range hint
311-323: Exceeding file count
Ensures the error is thrown for too many files. Proper coverage of constraints.
Line range hint
326-337: Invalid file type
Catches file type mismatches. Confirmed to throwInvalidFileType.vitest.workspace.ts (2)
1-2: Workspace definition import
Bringing indefineWorkspacefromvitest/configis standard for advanced Vitest setups.
3-28: Two-workspace configuration
Separating unit vs. browser tests is good to isolate environment dependencies:
- Unit tests run in Node environment
- Browser tests operate with Playwright config
This is a clean approach to handle distinct test flavors.packages/react/test/upload-dropzone.browser.test.tsx (6)
1-3: Browser-based test context
Imports from@vitest/browser/contextandmsw/browseralign with the new approach.
33-33: Mock worker
Similar to the upload-button test, this worker setup is consistent. Good job consolidating.
51-52: beforeAll/afterAll worker
Same pattern as before. No concerns.
65-67: Ready state check
Ensures component moves from “Loading…” to “ready.” Correct usage ofvi.waitFor.
72-72: Visibility check
Confirming the label text is visible. This is consistent with the approach inupload-button.browser.test.tsx.
85-87: Plural label
Checks that the label changes for multiple file acceptance. Looks correct.packages/uploadthing/test/__test-helpers.ts (6)
5-5: Mock usage
viimport for mocks is standard.
31-41: Process-based fallback
Usingtypeof process !== "undefined"ensures SSR or browser contexts don’t break. The environment fallback logic is valuable.
81-88: handlers array
Consolidating handlers into one export is a clean approach for MSW usage.
89-92: UTFS content
Thehttp.gethandler returns a fixed text response forUTFS_IO_URLcalls—straightforward.
96-110: UploadThing Ingest
Constructs the file hash from request data and returns the correct JSON. This is a neat approach to ensuring data integrity in tests.
114-124: UploadThing API routes
Handlers forrequestFileAccessandupdateACLendpoints respond with minimal JSON, capturing necessary data for tests.packages/react/test/client-generator.test.ts (1)
1-1: Removing the unuseddescribeimport is appropriate.
This line now imports onlyexpectTypeOfanditfromvitest. Sincedescribeis not used in this file, removing it keeps the imports list lean.packages/uploadthing/test/sdk.live.test.ts (1)
288-288: Usage info mismatch between expected and actual.
The test expects an appTotalBytes of 0 and a filesUploaded count of -1, but the actual usage info shows non-zero values. Verify that the usage counters are reset or the environment is cleared before running tests, or adjust the expectation if the test environment is intended to be stateful.🧰 Tools
🪛 GitHub Check: build
[failure] 288-288: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should have correct usage info
AssertionError: expected GetUsageInfoResponse{ …(4) } to deeply equal { totalBytes: +0, …(3) }
- Expected
- Received
- Object {
- "appTotalBytes": 0,
- "filesUploaded": -1,
- GetUsageInfoResponse {
- "appTotalBytes": 15418,
- "filesUploaded": 5,
"limitBytes": 2147483648,
- "totalBytes": 0,
- "totalBytes": 15418,
}❯ packages/uploadthing/test/sdk.live.test.ts:288:25
🪛 GitHub Actions: CI
[error] 288-288: Test failure: 'should have correct usage info' - Mismatched values for appTotalBytes (expected: 0, got: 15418) and filesUploaded (expected: -1, got: 5)
packages/uploadthing/test/request-handler.test.ts (18)
3-3: Added import fordescribefrom Vitest.
This addition is consistent with the usage ofdescribeblocks below.
Line range hint
80-91: Ensuring 404 for invalid slug.
This test properly checks if the slug is invalid and should 404. No issues spotted.
Line range hint
98-118: Verifying error handling for invalid action type.
The test ensures a 400 error is returned for unsupported action types. Good coverage.
Line range hint
120-138: Rejecting unmatched file types.
The test confirms that a.txtfile is blocked by theimageUploaderconfig. This looks correct.
Line range hint
139-160: Rejecting oversized files.
The test blocks a file that exceeds the2MBthreshold. The logic is correct.
Line range hint
161-183: Limiting file count to 1.
Checks that uploading 2 files triggers aFileCountMismatch. Implementation is valid.
Line range hint
184-203: Checking minimum file count.
The test ensures you can’t upload fewer files thanminFileCount. This is good coverage.
Line range hint
204-229: Validating mime type vs. file extension.
Correctly prioritizesfile.typefor router matching. This approach is consistent with user agent behavior, where thetypefield is typically reliable.
Line range hint
230-255: Usingfile.typeoverfile.name.
Good to see consistent application of the same logic. This ensures matched file type is accurate.
Line range hint
256-284: Handling unrecognized mime types by file extension.
Falls back to “filename-based” type detection. This is a helpful fallback.
Line range hint
285-307: Blocking missing input.
This test checks missinginputfor routes requiring it. The error structure is well-defined.
Line range hint
308-333: Blocking invalid schema input.
Ensures an error when ‘foo’ doesn’t match the enum. Coverage is good.
Line range hint
334-355: Confirming input is forwarded to middleware.
Tests that the input is handed off, verifying the shape and data. Implementation looks correct.
Line range hint
356-376: Confirming files are forwarded to middleware.
Verifies the file list is passed along. No issues noted.
Line range hint
377-406: Middleware throws early.
Checks that the request is terminated if the middleware fails. This is important for robust error handling.
Line range hint
407-456: Ensuring correct arguments toonUploadComplete.
This test confirms that the route’s completion hook is called with the correct data. Great coverage.
Line range hint
457-489: Blocking missing signature.
Confirms a 400 error whenx-uploadthing-signatureis absent. Implementation is correct.
Line range hint
490-506: Blocking invalid signature.
Verifies it rejects the request if the signature is invalid. Security coverage is good.packages/uploadthing/test/sdk.test.ts (27)
2-2: Using “msw/node” for request mocking.
This approach is consistent with a Node-based test environment. No concerns here.
9-9: Importing testing utilities from Vitest.
Nothing to add, it’s consistent with the rest of the test suite.
16-16: Addinghandlersfrom__test-helpers.
Indicates that the mock server will intercept requests to simulate the backend. This is standard for MSW.
23-26: Starting and stopping the MSW server.
This ensures the mock server is available for tests and stops it afterward. Good practice.
Line range hint
30-62: Verifying single-file uploads.
Ensures request structure is correct for file uploads. No issues spotted.
63-69: Returning an array of results.
Confirms that passing an array of files results in an array ofUploadFileResult. Clear and correct.
70-76: Returning a single object.
Ensures a singleFileleads to a single result. Good coverage.
Line range hint
77-88: AcceptingUTFile.
Validates custom UTFile usage. Implementation matches design.
89-95: Accepting Undici’sFile.
Verifies cross-environment file type support.
Line range hint
96-109: Handling customId inUTFile.
Ensures the custom ID is properly inserted into the request.
Line range hint
110-145: Downloading and re-uploading from a URL.
Mimics a remote file download before uploading, verifying both requests. Implementation is thorough.
Line range hint
146-155: Array-baseduploadFilesFromUrl.
Testing multiple URLs returns an array. No concerns noted.
Line range hint
156-164: Single URL returns a single object.
Consistent with the rest of the method’s design.
Line range hint
165-183: Overriding file name.
Confirms that you can rename the file when uploading from a URL. Useful feature.
Line range hint
184-203: Using a customId with URL upload.
Ensuresx-ut-custom-idis included. Implementation is consistent.
Line range hint
204-219: Returning error for data URLs.
Preventing inline data URLs from this method is logical. Good constraint.
Line range hint
220-245: Handling download errors gracefully.
Demonstrates robust handling of 404 and other non-2xx responses. Good approach.
Line range hint
246-289: Preserving partial successes in array uploads.
The second test block also checks an existing file followed by a 404. Properly returning an array with mixed success/failure. Good job.
Line range hint
290-341: Maintaining result order even if some downloads fail.
Ensures that partial success/failure can be matched to the correct URLs.
Line range hint
342-358: ValidatinggetSignedURLwithoutexpiresIn.
Ensures the correct request body is sent. No concerns.
Line range hint
359-375: TestingexpiresInwith “1d”.
Confirms a valid time string is converted to seconds. Implementation is correct.
Line range hint
376-392: TestingexpiresInwith “3 minutes”.
Checks conversion to 180 seconds. Works well.
Line range hint
393-403: Handling invalidexpiresInstrings.
Ensures an error is thrown for nonsensical durations. Good validation.
Line range hint
404-415: Rejecting durations beyond the 7-day maximum.
Prevents overly long access times. Implementation is consistent with the product constraints.
Line range hint
416-434: Update ACL for a single file.
Checks the request body and success response. Straightforward.
Line range hint
435-458: Update ACL for multiple file keys.
Ensures the array is processed and requests are correct. Good approach.
Line range hint
459-479: Override keyType to “customId” in updateACL.
Verifies thatcustomIdis used instead offileKey. Implementation is correct.packages/uploadthing/test/adapters.test.ts (2)
15-24: No issues found in the updated imports and test lifecycle setup.
All MSW and vitest lifecycle hooks appear correctly configured and consistent with best practices.
29-29: Potential Redundant Import
baseHeadersis imported and used, but ensure it is still relevant for all test cases. Consider removing if no longer needed.examples/backend-adapters/package.json (1)
26-26: Dependabot or Similar Tools
Upgrading@playwright/testto1.49.1looks great. Consider enabling automated checks (via Dependabot or Renovate) to keep it updated in the future.
[approve]package.json (1)
60-60: Upgrade Vitest for Enhanced Features
vitestnow at^2.1.8includes better module resolution and coverage. Ensure your coverage thresholds/tests are updated to leverage these new features.packages/react/package.json (1)
92-92: Validate Compatibility ofvitest-browser-react
New dependencyvitest-browser-reactis introduced at version0.0.4. Ensure it is compatible with the existing test setup and library versions.✅ Verification successful
Let me gather more information about the test setup and dependencies.
Let me check the vitest configuration and browser test setup.
vitest-browser-reactis Compatible with Current SetupThe dependency is being used correctly in browser-based tests and is compatible with the current setup:
- It's properly integrated with Vitest configuration
- Used in browser test files for rendering React components
- Works with MSW for API mocking
- Compatible with the project's React version (18.3.1)
- Tests are passing with the expected functionality
The version 0.0.4 is working as intended with no compatibility issues found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how 'vitest-browser-react' is imported/required across the codebase rg -A 3 'vitest-browser-react'Length of output: 2440
Script:
#!/bin/bash # Check package.json for test-related dependencies and configurations cat packages/react/package.json | jq '.dependencies,.devDependencies,.peerDependencies,.scripts' # Check for vitest configuration fd vitest.configLength of output: 1420
Script:
#!/bin/bash # Check vitest configuration files cat vitest.config.ts packages/svelte/vitest.config.ts # Check browser test files that use vitest-browser-react cat packages/react/test/upload-dropzone.browser.test.tsx packages/react/test/upload-button.browser.test.tsxLength of output: 13788
.github/workflows/ci.yaml (1)
Line range hint
37-42: Verify test token permissions for browser tests.Since browser-based tests with MSW are being added, ensure that the
UPLOADTHING_TEST_TOKENhas sufficient permissions for the new test scenarios.
| const file = new File(["foo"], "foo.txt", { type: "text/plain" }); | ||
| const result = await utapi.uploadFiles(file); | ||
| const key = result.data!.key; | ||
| expect(result).toEqual({ |
There was a problem hiding this comment.
Mismatch between expected “staging” domain and received domain.
These tests are failing in the pipeline because the actual file URLs differ from the expected “staging.utfs.io” domain. You could fix this by either updating the environment/test configuration to use the correct domain or adjusting these test expectations to match the production domain.
Also applies to: 101-101, 132-132, 157-157, 201-201, 242-242
🧰 Tools
🪛 GitHub Check: build
[failure] 65-65: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should upload a file
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89I5iY5NnbUlVkevf5bcZGNoLy26pw7XI3ODmjR", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919930323, "name": "foo.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89I5iY5NnbUlVkevf5bcZGNoLy26pw7XI3ODmjR",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:65:22
🪛 GitHub Actions: CI
[error] 65-65: Test failure: 'should upload a file' - URL mismatch between expected staging.utfs.io and received utfs.io domains
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
packages/uploadthing/test/sdk.test.ts (2)
30-30: Coverage for file upload success
The test named"uploads successfully"verifies a smooth flow. Consider adding a scenario to ensure the new custom fetch override also triggers or integrates as expected.
246-246: Second download error test
This test is nearly identical in purpose to the prior download error scenario. Consider merging or renaming to remove duplication.package.json (1)
50-53: New and updated test libraries
The addition of@vitest/browser,@vitest/coverage-v8,[email protected], and[email protected]reflect a shift to more comprehensive browser and coverage testing. Ensure that these versions match any frameworks or plugin compatibilities within your pipelines..github/workflows/ci.yaml (1)
34-36: LGTM! Consider caching Playwright browsers.The addition of Playwright installation is well-positioned in the workflow. Installing only Chromium is an efficient choice for CI.
Consider adding browser caching to speed up CI runs:
- name: Install Playwright + id: playwright-install + if: steps.playwright-cache.outputs.cache-hit != 'true' run: pnpm exec playwright install chromiumpackages/uploadthing/test/client.browser.test.ts (1)
Line range hint
35-111: Consider adding error handling for worker setup.The setupUTServer implementation looks good, but there's room for improvement in error handling.
Consider adding error handling for worker setup:
const port = Math.floor(Math.random() * 10000) + 10000; const url = `http://localhost:${port}`; -worker.use(...handlers, http.all(`${url}/api/uploadthing`, handler)); +try { + worker.use(...handlers, http.all(`${url}/api/uploadthing`, handler)); +} catch (error) { + console.error('Failed to setup MSW worker:', error); + throw error; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/ci.yaml(1 hunks)examples/backend-adapters/package.json(1 hunks)package.json(1 hunks)packages/react/package.json(1 hunks)packages/react/test/client-generator.test.ts(1 hunks)packages/react/test/upload-button.browser.test.tsx(12 hunks)packages/react/test/upload-dropzone.browser.test.tsx(5 hunks)packages/react/vitest.config.ts(0 hunks)packages/uploadthing/test/__test-helpers.ts(3 hunks)packages/uploadthing/test/adapters.test.ts(16 hunks)packages/uploadthing/test/client.browser.test.ts(10 hunks)packages/uploadthing/test/request-handler.test.ts(18 hunks)packages/uploadthing/test/sdk.live.test.ts(1 hunks)packages/uploadthing/test/sdk.test.ts(20 hunks)packages/uploadthing/test/upload-builder.test.ts(0 hunks)packages/uploadthing/vitest.config.ts(0 hunks)patches/@[email protected](0 hunks)patches/[email protected](0 hunks)vitest.workspace.ts(1 hunks)
💤 Files with no reviewable changes (5)
- packages/uploadthing/test/upload-builder.test.ts
- packages/uploadthing/vitest.config.ts
- packages/react/vitest.config.ts
- patches/@[email protected]
- patches/[email protected]
🧰 Additional context used
🪛 GitHub Check: build
packages/uploadthing/test/sdk.live.test.ts
[failure] 65-65: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should upload a file
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89I5iY5NnbUlVkevf5bcZGNoLy26pw7XI3ODmjR", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919930323, "name": "foo.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89I5iY5NnbUlVkevf5bcZGNoLy26pw7XI3ODmjR",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:65:22
[failure] 101-101: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should upload a private file
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89IIPJue9MGkZXF0NudKOg2JioQU9fSrqEHmAIt", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919932843, "name": "foo.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89IIPJue9MGkZXF0NudKOg2JioQU9fSrqEHmAIt",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:101:22
[failure] 132-132: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should upload a file from a url
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89INBGOdPauf9W1zAX4dnGIJywloj3qi780TYgP", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": Any<Number>, "name": "favicon.ico", "size": Any<Number>, "type": "image/vnd.microsoft.icon",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89INBGOdPauf9W1zAX4dnGIJywloj3qi780TYgP",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:132:22
[failure] 157-157: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should rename a file with fileKey
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89I2ybL6RiGHj7eSFgd68wM9or4mx2JYyqIBzTU", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": Any<Number>, "name": "bar.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89I2ybL6RiGHj7eSFgd68wM9or4mx2JYyqIBzTU",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:157:22
[failure] 201-201: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should rename a file with customId
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { data: { …(9) }, error: null }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89IaOBjqaDBFgnWez4rqkHEVGPhb0sCfNOyQUjZ", "customId": "ba44ad17-0e0f-466b-9bb7-39d58f74bed3", "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919938995, "name": "bar.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89IaOBjqaDBFgnWez4rqkHEVGPhb0sCfNOyQUjZ",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:201:22
[failure] 242-242: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should update ACL
AssertionError: expected { data: { …(9) }, error: null } to deeply equal { Object (data, error) }
- Expected
-
Received
Object {
"data": Object {
-
"appUrl": StringMatching /^https:\/\/staging.utfs.io\/a\/fr0hfwpst1\/.+$/,
-
"appUrl": "https://utfs.io/a/fr0hfwpst1/zIptSc0cA89I364mLTEXFuHUGrj5WXe3Cn0bx6fPNozmBAZd", "customId": null, "fileHash": Any<String>, "key": StringMatching /.+/, "lastModified": 1735919941005, "name": "foo.txt", "size": 3, "type": "text/plain",
-
"url": StringMatching /^https:\/\/staging.utfs.io\/f\/.+$/,
-
},
"url": "https://utfs.io/f/zIptSc0cA89I364mLTEXFuHUGrj5WXe3Cn0bx6fPNozmBAZd",
"error": null,
}
❯ packages/uploadthing/test/sdk.live.test.ts:242:22
[failure] 288-288: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should have correct usage info
AssertionError: expected GetUsageInfoResponse{ …(4) } to deeply equal { totalBytes: +0, …(3) }
- Expected
- Received
- Object {
- "appTotalBytes": 0,
- "filesUploaded": -1,
- GetUsageInfoResponse {
- "appTotalBytes": 15418,
- "filesUploaded": 5,
"limitBytes": 2147483648,
- "totalBytes": 0,
- "totalBytes": 15418,
}
❯ packages/uploadthing/test/sdk.live.test.ts:288:25
🪛 GitHub Actions: CI
packages/uploadthing/test/sdk.live.test.ts
[error] 65-65: Test failure: 'should upload a file' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 101-101: Test failure: 'should upload a private file' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 132-132: Test failure: 'should upload a file from a url' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 157-157: Test failure: 'should rename a file with fileKey' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 201-201: Test failure: 'should rename a file with customId' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 242-242: Test failure: 'should update ACL' - URL mismatch between expected staging.utfs.io and received utfs.io domains
[error] 288-288: Test failure: 'should have correct usage info' - Mismatched usage statistics (expected 0 bytes, got 15418 bytes)
🔇 Additional comments (99)
packages/uploadthing/test/__test-helpers.ts (2)
31-33: Consider verifying environment variable references.
This block relies onprocess.env.UPLOADTHING_API_URL, which may not be populated or might differ between environments, potentially leading to unexpected defaults.Would you like a script to check where and how
UPLOADTHING_API_URLis defined or used in other parts of the code?
81-124: Handlers look consistent with your use of UTFS_IO_URL and INGEST_URL.
The mocking structure is straightforward and maintainable. Good job!packages/uploadthing/test/sdk.live.test.ts (1)
288-288: Usage info mismatch in tests.
The usage info is off by several bytes and files. Verify that your cleanup steps or local usage tracking logic match reality.Would you like a script to examine references to
localInfoor confirm that yourdeleteFileslogic fully removes usage data?🧰 Tools
🪛 GitHub Check: build
[failure] 288-288: packages/uploadthing/test/sdk.live.test.ts > smoke test with live api > should have correct usage info
AssertionError: expected GetUsageInfoResponse{ …(4) } to deeply equal { totalBytes: +0, …(3) }
- Expected
- Received
- Object {
- "appTotalBytes": 0,
- "filesUploaded": -1,
- GetUsageInfoResponse {
- "appTotalBytes": 15418,
- "filesUploaded": 5,
"limitBytes": 2147483648,
- "totalBytes": 0,
- "totalBytes": 15418,
}❯ packages/uploadthing/test/sdk.live.test.ts:288:25
🪛 GitHub Actions: CI
[error] 288-288: Test failure: 'should have correct usage info' - Mismatched usage statistics (expected 0 bytes, got 15418 bytes)
packages/react/test/upload-button.browser.test.tsx (22)
1-5: Switch tomsw/browserand new testing utilities
The imports now rely on@vitest/browser/contextandvitest-browser-react. This transition is consistent with the browser-based test strategy and should improve test reliability and clarity.
37-37: Worker setup
UsingsetupWorkerfor MSW is correct for browser-based tests. This aligns with the new test environment configuration.
55-56: Proper lifecycle hooks for MSW
Callingworker.start()inbeforeAllandworker.stop()inafterAllensures that the mock worker is correctly running before tests and shut down after.
98-100: Appropriate wait logic
Awaitingvi.waitForensures the label transitions to"ready". This appears correct and prevents flaky tests due to asynchronous rendering.
105-105: Element visibility check
Theawait expect.element(page.getByText("Image (4MB)")).toBeVisible()assertion is a good approach to confirm UI presence.
120-122: Consistent loading check
Reusingvi.waitForto confirm"ready"state is consistent with other tests, ensuring reliability in UI transitions.
127-127: Visibility assertion
Confirming"Image (4MB)"presence here is consistent and ensures the route config was fetched.
140-142: Ready state verification
Again,vi.waitForis used consistently to validate the readiness of the label.
150-150: UI text check
Ensuring"Image (4MB)"is visible validates that the label is updated as expected.
159-161: Confirming label attribute
This pattern is repeated effectively across tests to avoid premature assertions.
163-165: User event file upload
UsinguserEvent.uploadis recommended overfireEvent, providing a more realistic user interaction simulation.
174-174: Increased file size
The change from3to18ensures a more realistic scenario or modifies the test scope. Verify any relevant references.
189-191: Asynchronous state readiness check
No issues noted. This ensures the label has updated to"ready".
193-195: Manual mode - file upload
Uploading an empty file is a valid test scenario for edge cases. Looks good.
198-199: Manual upload action
Clicking the label to triggeronUploadBeginis consistent with the manual upload flow.
214-214: Text content validation
Verifying"Choose File"is displayed ensures the default state is rendered properly.
230-230: Confirm default label
Ensuring label visibility helps validate the fallback or initial UI.
232-235: User event upload
This verifies that the correct post request logic is triggered upon file selection.
252-258: Multiple files
Uploading multiple files tests the maxFileCount scenario. The approach is consistent.
277-280: Style assertion
Checking the background color ensures custom appearance overrides are functioning correctly.
287-287: Custom content render
Providing content props is tested by rendering a custom string. No issues noted.
295-295: Custom content visibility
Ensuring"Allowed content"is visible validates dynamic text rendering.vitest.workspace.ts (2)
1-2: LeveragedefineWorkspace
Importing and usingdefineWorkspacefromvitest/configis the recommended approach for multi-project setups.
3-28: Separate test configurations
Defining two configurations for "unit" and "browser" tests clarifies which tests run in each environment. This separation is a best practice for complex test suites.packages/react/test/upload-dropzone.browser.test.tsx (6)
1-7: Transition tomsw/browser
These import adjustments align with the move to browser-based mocks and thevitest-browser-reactenvironment.
33-33: Worker initialization
const worker = setupWorker(...)is correct for browser test contexts and ensures the interceptors function as expected.
51-52: Start/stop worker
UsingbeforeAll(() => worker.start())andafterAll(() => worker.stop())prevents potential test conflicts from leftover handlers.
65-67: Wait for readiness
vi.waitForusage is consistent, ensuring asynchronous transitions settle before assertions.
72-72: Label text check
Checking"Image (4MB)"confirms the route config is displayed properly.
85-87: File count state
Similar readiness check for multiple file handling. No issues.packages/react/test/client-generator.test.ts (1)
1-1: Updated imports
Removingdescribewhile keepingitis fine, provided no grouped test blocks are needed. The test logic remains intact.packages/uploadthing/test/request-handler.test.ts (18)
3-3: Unified import for Vitest
Importingdescribe,expect, anditfrom"vitest"standardizes the test structure.
80-80: Removeddbargument
Ensuringdbwas unused. This change simplifies the signature. Confirm no upstream references requiredb.
98-98: Trimmed test parameters
Similar removal of({ db }). No functional issue seen.
120-120: File type blocking
Positioned to check invalid file types. Removal ofdbremains consistent.
139-139: Handling max file size
Ensuring the test doesn't rely on an externaldb. This remains correct.
161-161: Multi-file limit
No changes in logic beyond param removal. Looks good.
184-184: Minimum file count
Again, droppingdbappears to have no effect on the test.
204-204: Mime type matching
Remains functionally consistent withoutdb.
230-230: Prioritized file.type
Handling was tested thoroughly before. No regression from removingdb.
256-256: Fallback to filename
Behavior is verified. No concerns with parameters.
285-285: Missing input
dbremoval has no direct impact. Implementation remains correct.
308-308: Schema mismatch
No additional concerns.
334-334: Middleware input
Still validated properly.
356-356: Forwarding files
No effect from removingdb.
377-377: Middleware throw
Still tested thoroughly.
407-407: Upload complete handler
Checks remain functional. No issues.
457-457: Missing signature
Logic unaffected by parameter removal.
490-490: Invalid signature
Test coverage remains intact.packages/uploadthing/test/sdk.test.ts (25)
2-2: Introduced MSW setup import
ImportingsetupServerfrommsw/nodeis a good choice to mock network requests in a Node environment. This helps ensure that tests remain isolated and do not depend on real external services.
9-9: Additional named import from Vitest
The import ofitfrom Vitest is consistent with the test style used throughout the file.
16-16: Usage ofAPI_URL
The additional import ofAPI_URLindicates your tests may rely on a shared base URL for request assertions. Keep an eye on test reliability if referencing external or dynamic endpoints.
23-25: MSW server initialization
Setting up the MSW server withsetupServer(...handlers), and callingmsw.listen()/msw.close()before and after tests, is an excellent approach for controlling and resetting request handlers. Just remember to reset any request handlers if you add custom overrides at test runtime.
63-63: Array return test
Testing behavior when passing an array of files ensures type consistency. Looks good.
70-70: Single object return test
Ensuring a single file returns an object avoids confusion for consumers. Good coverage.
77-77: UTFile acceptance test
Validates usage of your special UTFile class. The test confirms it integrates seamlessly with the upload logic.
89-89: UndiciFile acceptance test
Ensures interoperability with the UndiciFileclass. Good for cross-environment compatibility.
96-96: Custom ID support
ValidatingUTFilewith acustomIdhelps confirm advanced metadata handling. Looks good.
110-110: Download then upload test
Ensures that remote file fetching and subsequent upload flows are correct. Consider verifying custom fetch usage in this scenario as well.
146-146: Array pass-through test
Confirmation that multiple URLs produce multiple results. Keeping tests granular is beneficial.
156-156: Single item pass-through test
Checks single remote URL uploading. Straightforward coverage—no issues noted.
165-165: Name override test
Passing a custom name helps ensure correct file labeling. Verify if newfetchoverride could alter name or headers.
184-184: Custom ID via URL test
Tests thatcustomIdflows through to the server for remote URLs. Nicely done.
204-204: Data URL rejection test
Good job restrictinguploadFilesFromUrlto remote resources only. This clarifies usage for library consumers.
220-220: Error handling test for missing remote files
Validates the library’s resilience when a download fails. Good coverage of negative cases.
290-290: Preservation of order on partial failures
Your test ensures partial success scenarios keep the original order. This is an important detail for client-side consistency.
342-342: Unsigned request test
Validates that an omittedexpiresInleads to the default behavior, ensuring no unexpected parameter usage.
359-359: Valid expiresIn (1d)
Checks correct numeric conversion for date strings. This is helpful for time-based validations.
376-376: Valid expiresIn (3 minutes)
Another robust check, verifying fractional day conversions. Nice thoroughness.
393-393: Invalid expiresIn
Ensures that nonsense/invalid user input is quickly rejected. Great user safety net.
404-404: Maximum expiresIn
Verifies the system enforces a 7-day limit, preventing overly long signed URLs. Good boundary test.
416-416: ACL update for single file
Straightforward ACL update test. Clear approach to verifying permission changes.
435-435: ACL update for multiple keys
Expanding ACL changes to multiple files at once. Looks succinct and correct.
459-459: Keytype override in ACL
EnsurescustomIdusage with ACL updates. This test helps confirm advanced usage scenarios.packages/uploadthing/test/adapters.test.ts (16)
15-24: Introduced MSW server
UsingsetupServerfrommsw/nodefor request mocking and hooking intobeforeAll/afterAllis consistent with your overall testing strategy. This should streamline adapter tests by eliminating the need for real network calls.
60-60: H3 adapter: router configuration test
Ensures the adapter responds with the correct router config on GET. Good coverage for the H3 adapter’s metadata.
82-82: H3 adapter: access to H3Event
Confirms the adapter passes the correct event type to the middleware. Great for framework-specific integration.
171-171: Server adapter: router configuration test
Similar to the H3 test, verifying the GET route returns the expected structure. Clear approach.
191-191: Server adapter: access to Request
Checksreqavailability in middleware. Good coverage to confirm server adapter usage.
249-249: Server adapter: object with request
Ensures the handler can accept an object form including therequestproperty, adding more flexibility for the caller.
326-326: Next.js App Router: router configuration test
Ensures router config is retrievable under the Next.js App Router format. This is consistent with other adapters.
348-348: Next.js App Router: NextRequest in middleware
Validates that the NextRequest type is passed properly. This consistency is important for SSR or edge use cases.
461-461: Next.js Legacy: router GET config
Ensures the older Next.js Pages Router works similarly. Good backward compatibility check.
485-485: Next.js Legacy: NextApiRequest and NextApiResponse
Verifies that older Next.js APIs receive bothreqandres, ensuring correct middleware usage.
584-584: Express adapter: router configuration test
Ensures the base route returns the correct adapter config. Good baseline coverage for express.
602-602: Express adapter:reqandres
Checks that the middleware receives theexpress.Requestandexpress.Responseobjects. Excellent for verifying typical usage in Node express.
662-662: Express adapter: built-in middlewares test
Confirms that standard body parsing or JSON middlewares do not interfere with your logic. Good interoperability check.
682-682: Express adapter: body-parser middleware
Similar to the standard built-in middlewares test, ensuring third-party or legacy middlewares integrate seamlessly.
738-738: Fastify adapter: router config test
Ensures the route is returning correct config on GET. Maintains parallel coverage with other adapters.
756-756: Fastify adapter: FastifyRequest and FastifyReply
Verifies the fastify adapter passes the proper request/response objects to your middleware. Critical for type-safe usage.examples/backend-adapters/package.json (1)
26-26: Playwright version bump
Updating@playwright/testto1.49.1aligns with the changes in the root package. Make sure the new version matches any pinned versions in your CI configuration and that tests pass reliably.package.json (1)
60-60: Vitest version update
Moving from^2.1.2to^2.1.8can bring performance improvements and bug fixes. Retest old or critical suites to confirm no regressions.packages/react/package.json (1)
92-92: Verify the stability of vitest-browser-react package.The package
[email protected]is in a very early version (0.0.x), which might indicate potential instability.✅ Verification successful
Let me gather more information about the package usage and its dependencies.
The package appears to be stable despite its early version
While the version number (0.0.4) might suggest early development, several factors indicate stability:
- The package is maintained by the Vitest team (vitest-dev organization)
- It has regular updates with the latest release being a month ago
- There are no open bug reports
- It has zero dependencies, reducing potential stability issues
- It's a small, focused package (9.3 kB unpacked) that serves a single purpose
- It's published under the MIT license with proper documentation
The version number appears to be more indicative of the package's early lifecycle rather than instability concerns. The package should be safe to use in its current form.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the package's release history and GitHub activity gh api -X GET repos/$(npm view vitest-browser-react repository.url | sed 's/.*github.com\/\(.*\)\.git/\1/')/releases | jq -r '.[].published_at' | sort -r | head -n 5 # Check for any reported issues gh api -X GET search/issues -f q="repo:$(npm view vitest-browser-react repository.url | sed 's/.*github.com\/\(.*\)\.git/\1/') is:issue is:open label:bug" | jq '.total_count'Length of output: 494
Script:
#!/bin/bash # Check package details including dependencies and maintenance info npm view vitest-browser-react # Check if there are any breaking changes or major issues in the changelog rg -i "breaking|deprecat|security" -g "CHANGELOG.md" --no-filename # Check the total number of dependent packages npm view vitest-browser-react dependenciesLength of output: 4575
packages/uploadthing/test/client.browser.test.ts (3)
1-33: LGTM! Good choice using MSW for request mocking.The switch to MSW (Mock Service Worker) is a solid improvement:
- Provides more realistic request interception
- Better matches browser behavior
- Enables testing of custom fetch implementations
245-261: LGTM! Good test coverage for custom fetch implementation.The test case for custom fetch implementation is well-structured:
- Verifies that the custom fetch is called
- Maintains original functionality by delegating to window.fetch
- Properly cleans up resources
Line range hint
264-289: Consider implementing retry tests.The commented-out retry test suggests this functionality might be needed. Consider implementing retry logic for better reliability.
Would you like me to help implement the retry test case and the underlying retry mechanism?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
.changeset/nice-penguins-grow.md (1)
10-10: Consider enhancing the changeset description.While the description accurately indicates the new feature, it would be helpful to include more details about the feature's benefits and use cases.
-feat: allow custom fetch override +feat: allow custom fetch override for configurable HTTP clients + +This change enables users to provide their own fetch implementation, +allowing for custom HTTP clients, better testing, and enhanced control +over the upload process.packages/svelte/src/lib/component/UploadDropzone.svelte (3)
29-29: LGTM! Consider documenting internal API usage.The rename to
__createUploadThingInternalbetter reflects its internal nature. However, since this is an internal API, consider adding a comment explaining why external consumers should not use it directly.+// @internal - This is an internal API and should not be used directly import { __createUploadThingInternal } from "../create-uploadthing";
83-86: LGTM! Consider enhancing type safety for the fetch override.The fetch override implementation with fallback to
globalThis.fetchis clean and follows the pattern used across other framework implementations.Consider adding type safety for the fetch override to ensure it matches the expected fetch API signature:
- uploader.fetch ?? globalThis.fetch, + (uploader.fetch as typeof fetch) ?? globalThis.fetch,
Line range hint
51-56: Enhance deprecation notice with migration timeline.While the deprecation is properly documented, users would benefit from more guidance on migration.
Enhance the deprecation notice with more details:
/** * Callback called when files are dropped or pasted. * * @param acceptedFiles - The files that were accepted. - * @deprecated Use `onChange` instead + * @deprecated Use `onChange` instead. This callback will be removed in the next major version. + * @example + * // Before + * <UploadDropzone onDrop={(files) => console.log(files)} /> + * // After + * <UploadDropzone onChange={(files) => console.log(files)} /> */packages/react/src/useUploadThing.ts (2)
Line range hint
129-134: Consider enhancing error reportingGood addition of detailed error cause logging. Consider structuring the error information to make it more machine-readable for error monitoring systems.
- console.error( - "Something went wrong. Please contact UploadThing and provide the following cause:", - error.cause instanceof Error ? error.cause.toString() : error.cause, - ); + console.error({ + message: "UploadThing encountered an error", + errorCause: error.cause instanceof Error ? error.cause.toString() : error.cause, + timestamp: new Date().toISOString(), + });
Line range hint
162-175: LGTM: React helpers fetch customizationThe fetch customization is well-implemented with appropriate fallback to global fetch. Consider adding JSDoc comments to document the fetch parameter usage.
const generateReactHelpers = <TRouter extends FileRouter>( + /** + * @param initOpts.fetch - Optional custom fetch implementation + * @param initOpts.url - Optional custom URL for the upload endpoint + */ initOpts?: GenerateTypedHelpersOptions, ) => {packages/solid/src/utils/createFetch.ts (1)
31-31: Consider cache key strategy for custom fetch implementations.The cache key is solely based on the URL, but custom fetch implementations might modify the request in ways that should affect caching (e.g., adding headers or transforming the URL). Consider including relevant request properties in the cache key or allowing cache key customization.
Example implementation:
interface CreateFetchOptions<T> extends RequestInit { getCacheKey?: (url: string, options?: RequestInit) => string; } export function createFetch<T = unknown>( fetchFn: FetchEsque, url?: string, options?: CreateFetchOptions<T>, ) { const getCacheKey = options?.getCacheKey ?? ((u) => u); const cache: Cache<T> = {}; // In the resource creation: const cacheKey = getCacheKey(url!, options); if (cache[cacheKey]) { return { type: "fetched", data: cache[cacheKey] }; } // ... rest of the implementation }packages/svelte/package.json (1)
68-75: ESLint rules align well with custom fetch override feature.The
no-restricted-globalsrule forfetchis well-implemented and supports the PR's objective of allowing custom fetch implementations. The error message is clear and helpful.Consider making the
no-consolerule more flexible for development:- "no-console": "error", + "no-console": ["error", { "allow": ["warn", "error"] }],This would still prevent usage of
console.logwhile allowing important warnings and errors during development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/nice-penguins-grow.md(1 hunks).nvmrc(1 hunks)docs/package.json(2 hunks)examples/backend-adapters/server/package.json(1 hunks)package.json(1 hunks)packages/react/package.json(1 hunks)packages/react/src/components/index.tsx(3 hunks)packages/react/src/useUploadThing.ts(7 hunks)packages/react/test/client-generator.test.ts(3 hunks)packages/shared/package.json(1 hunks)packages/solid/package.json(1 hunks)packages/solid/src/components/button.tsx(2 hunks)packages/solid/src/components/dropzone.tsx(2 hunks)packages/solid/src/create-uploadthing.ts(1 hunks)packages/solid/src/utils/createFetch.ts(3 hunks)packages/svelte/package.json(1 hunks)packages/svelte/src/lib/component/UploadButton.svelte(2 hunks)packages/svelte/src/lib/component/UploadDropzone.svelte(2 hunks)packages/svelte/src/lib/create-uploadthing.ts(2 hunks)packages/uploadthing/package.json(1 hunks)packages/vue/package.json(1 hunks)packages/vue/src/components/button.tsx(2 hunks)packages/vue/src/components/dropzone.tsx(2 hunks)packages/vue/src/types.ts(3 hunks)packages/vue/src/useUploadThing.ts(2 hunks)playground-v6/package.json(1 hunks)playground/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- playground-v6/package.json
- playground/package.json
- .nvmrc
- examples/backend-adapters/server/package.json
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/react/test/client-generator.test.ts
- packages/svelte/src/lib/component/UploadButton.svelte
- packages/react/package.json
- packages/vue/src/components/button.tsx
- packages/solid/src/components/button.tsx
- packages/vue/src/types.ts
- packages/react/src/components/index.tsx
- docs/package.json
- packages/solid/src/components/dropzone.tsx
- packages/svelte/src/lib/create-uploadthing.ts
🧰 Additional context used
📓 Learnings (2)
packages/svelte/src/lib/component/UploadDropzone.svelte (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#980
File: packages/solid/src/create-uploadthing.ts:119-119
Timestamp: 2024-11-12T10:36:58.532Z
Learning: In this codebase, the naming conventions differ between frameworks to align with ecosystem practices:
- In React and Vue, functions are prefixed with `useX` (e.g., `useRouteConfig`) to match built-in hooks and conventions.
- In Solid and Svelte, functions are prefixed with `createX` (e.g., `createRouteConfig`) to align with their respective built-in functions and ecosystem conventions.
packages/solid/src/create-uploadthing.ts (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#980
File: packages/solid/src/create-uploadthing.ts:119-119
Timestamp: 2024-11-12T10:36:58.532Z
Learning: In this codebase, the naming conventions differ between frameworks to align with ecosystem practices:
- In React and Vue, functions are prefixed with `useX` (e.g., `useRouteConfig`) to match built-in hooks and conventions.
- In Solid and Svelte, functions are prefixed with `createX` (e.g., `createRouteConfig`) to align with their respective built-in functions and ecosystem conventions.
🔇 Additional comments (19)
packages/vue/src/useUploadThing.ts (3)
38-46: LGTM: Well-implemented fetch parameter handling.The useRouteConfig function correctly integrates the custom fetch implementation while maintaining type safety.
57-154: LGTM: Comprehensive error handling and progress tracking.The __useUploadThingInternal function is well-structured with proper error handling, progress tracking, and cleanup in the finally block.
165-172: LGTM: Proper fetch fallback implementation.The implementation correctly falls back to globalThis.fetch when no custom fetch is provided.
packages/vue/src/components/dropzone.tsx (2)
131-137: LGTM: Clean fetch parameter integration.The fetch parameter is correctly passed through the component hierarchy with proper fallback handling.
Line range hint
1-500: Consider adding unit tests for custom fetch scenarios.While the implementation looks solid, it would be beneficial to add tests that verify the custom fetch functionality works as expected.
Would you like me to help generate unit tests for custom fetch scenarios? These tests would verify:
- Custom fetch is properly called
- Fallback to globalThis.fetch works
- Error handling with custom fetch implementations
packages/react/src/useUploadThing.ts (4)
6-6: LGTM: Type import additionClean addition of the
FetchEsquetype import, necessary for the custom fetch override feature.
34-34: LGTM: Global fetch type declarationAppropriate type declaration for the global fetch function, ensuring type safety when accessing
globalThis.fetch.
62-68: LGTM: Internal upload function fetch customizationThe fetch parameter is correctly integrated into the internal upload function and properly passed to the uploader generator.
38-47: LGTM: Route config fetch customizationThe fetch parameter is correctly integrated into the route config retrieval logic. However, let's verify that all callers of this function have been updated to provide the fetch parameter.
✅ Verification successful
All useRouteConfig calls correctly provide the fetch parameter
The verification confirms that both calls to
useRouteConfigin the codebase (in React and Vue implementations) properly include the fetch parameter as the first argument, matching the updated function signature.
packages/react/src/useUploadThing.ts:144:useRouteConfig(fetch, url, _endpoint as string)packages/vue/src/useUploadThing.ts:141:useRouteConfig(fetch, url, _endpoint.value as string)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all useRouteConfig calls to ensure they provide the fetch parameter ast-grep --pattern 'useRouteConfig($$$)'Length of output: 270
packages/solid/src/utils/createFetch.ts (1)
4-5: LGTM! Good use of shared types.The import of
FetchEsquefrom the shared package promotes type consistency across the codebase.packages/solid/src/create-uploadthing.ts (4)
3-32: LGTM! Well-organized imports.The imports are properly structured and include all necessary types and utilities for the fetch customization feature.
34-41: Validate endpoint slugs before callingcreateRouteConfig.If
_endpointis not a valid slug,createRouteConfigcould returnundefined, causing downstream issues. Ensure proper validation or error handling to gracefully manage unexpected endpoint names.
43-136: Well-structured implementation with proper error handling and progress tracking!The function effectively manages upload state, progress tracking, and error handling while maintaining type safety through proper generic constraints.
138-167: Excellent implementation following Solid.js conventions!The function demonstrates several good practices:
- Follows Solid.js naming conventions with
useUploadThing(as noted in previous learnings)- Implements proper peer dependency validation
- Provides sensible fallback to global fetch when custom fetch is not provided
packages/shared/package.json (1)
39-39: Verify [email protected] compatibilityThe minor version bump of the
effectdependency might introduce changes relevant to the custom fetch override feature.Run the following script to check the changelog and potential breaking changes:
✅ Verification successful
The [email protected] update is safe to proceed
The version bump from 3.11.5 to 3.12.0 introduces several minor changes and patches, none of which affect or break the custom fetch override feature. The changes are primarily additions of new functionality:
- New encoding utilities (encodeUriComponent/decodeUriComponent)
- Runtime type extractors
- New schema utilities
- Cron improvements
- Context utilities
- Tracing enhancements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check effect package changelog for version 3.12.0 # Get the changelog or release notes for effect 3.12.0 gh api repos/Effect-TS/effect/releases/tags/[email protected] --jq .body || \ curl -s https://raw.githubusercontent.com/Effect-TS/effect/main/CHANGELOG.md | \ rg -A 10 "3.12.0"Length of output: 2917
package.json (1)
43-44: Verify testing infrastructure updatesThe significant updates to testing dependencies (@effect/vitest, @vitest/browser, @vitest/coverage-v8) and MSW suggest changes to the testing infrastructure. These changes might be related to testing the new custom fetch override feature.
Run the following script to check for any breaking changes in MSW that might affect tests:
Also applies to: 50-52
✅ Verification successful
MSW updates are safe to proceed with
Based on the changelog between MSW versions 2.2.13 and 2.7.0, there are no breaking changes that would affect the tests. The updates mainly include:
- New features: WebSocket API support (v2.6.0), HTML response helper (v2.4.0)
- Bug fixes and improvements: Headers handling, cookie management, request interception
- Dependencies updates and internal optimizations
The most significant change is the adoption of a new Socket-based request interception algorithm in v2.4.4, but this is an internal implementation detail that maintains backward compatibility.
Let's verify the test files to ensure they don't use any deprecated features:
MSW usage is compatible with the update
The test files show standard MSW usage patterns that are fully compatible with version 2.7.0:
- Browser tests use
setupWorkerwith basic request handlers- Node.js tests use
setupServerwith standard setup/teardown patterns- All tests use the stable HTTP API (
http,HttpResponse)No deprecated features or patterns are used, and the codebase follows MSW's recommended practices. The update from 2.2.13 to 2.7.0 is safe to proceed with, as the test infrastructure uses stable APIs that remain unchanged across these versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check MSW changelog for breaking changes between 2.2.13 and 2.7.0 gh api repos/mswjs/msw/releases --jq '.[] | select(.tag_name | contains("v2.")) | select(.tag_name >= "v2.2.13" and .tag_name <= "v2.7.0") | .body' || \ curl -s https://raw.githubusercontent.com/mswjs/msw/main/CHANGELOG.md | \ rg -A 10 "2.[2-7]"Length of output: 5574
Script:
#!/bin/bash # Check test files for MSW usage patterns rg -l "msw" --type=test # Then check the content of those files rg "msw|setupServer|setupWorker" --type=test -A 2 -B 2Length of output: 138
Script:
#!/bin/bash # Check for test files and MSW usage with proper file extensions rg -l "msw" -g "*.test.*" -g "*.spec.*" # Then check the content of those files rg "msw|setupServer|setupWorker" -g "*.test.*" -g "*.spec.*" -A 2 -B 2Length of output: 5200
packages/vue/package.json (1)
65-72: LGTM! ESLint rules enforce custom fetch override patternThe added ESLint rules effectively support the PR's objective:
- Preventing console usage improves code quality
- Restricting global fetch ensures developers use the new custom fetch override feature correctly
Let's verify if these rules are consistently applied across other framework packages:
✅ Verification successful
ESLint rules are consistently applied across framework packages
The verification confirms that both ESLint rules are uniformly implemented across all relevant framework packages:
The
no-restricted-globalsrule forfetchis present in:
- packages/react/package.json
- packages/solid/package.json
- packages/svelte/package.json
- packages/uploadthing/package.json
- packages/vue/package.json
The
no-console: "error"rule is also consistently applied in the same packages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if similar ESLint rules exist in other framework packages # Look for similar ESLint rules in other framework packages rg -U --multiline '(?s)"no-restricted-globals".*?"fetch".*?}' packages/*/package.json # Check for no-console rule rg '"no-console":\s*"error"' packages/*/package.jsonLength of output: 2836
packages/solid/package.json (1)
137-144: Consistent ESLint configuration across packages.The ESLint rules mirror those in the Svelte package, maintaining consistency across the codebase.
The same suggestion about making the
no-consolerule more flexible applies here as mentioned in the Svelte package review.packages/uploadthing/package.json (1)
154-158: Verify compatibility of effect library updates.The updates to
@effect/platform(0.70.7 → 0.72.0) andeffect(3.11.5 → 3.12.0) are minor version bumps. Let's verify these versions are compatible with the custom fetch override feature.Run the following script to check the changelog and release notes for any breaking changes:
✅ Verification successful
Let me gather more information about the effect library's usage in the codebase to ensure compatibility.
Dependencies update is safe to proceed
Based on the analysis of the codebase usage and the dependency updates:
- The
@effect/platformupdate (0.70.7 → 0.72.0) andeffect(3.11.5 → 3.12.0) are minor version bumps following semantic versioning.- The custom fetch override feature is implemented through:
FetchHttpClientlayer configuration inruntime.ts- Type-safe fetch implementation using
HttpClientinterfaces- All usages are through stable APIs that are not affected by these minor version updates
The codebase's usage of Effect libraries is focused on HTTP client/server abstractions which are stable features, and the custom fetch override implementation relies on core functionality that remains unchanged in these minor version updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check changelog/release notes for effect library updates # Check @effect/platform changelog gh api repos/Effect-TS/platform/releases \ --jq '.[] | select(.tag_name == "v0.72.0") | .body' # Check effect changelog gh api repos/Effect-TS/effect/releases \ --jq '.[] | select(.tag_name == "v3.12.0") | .body' # Search for any fetch-related changes in the codebase that might be affected rg -l "import.*@effect/platform|from.*@effect/platform" | while read -r file; do echo "=== $file ===" rg "fetch" "$file" || true doneLength of output: 2939
Script:
#!/bin/bash # Check for any Effect-specific types or features that might be affected ast-grep --pattern 'import { $$$ } from "@effect/platform"' # Check for any Effect-specific types or features that might be affected ast-grep --pattern 'import { $$$ } from "effect"' # Look for any FetchHttpClient usage rg "FetchHttpClient" -A 2 -B 2Length of output: 6371
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/solid/src/utils/createFetch.ts (2)
15-19: Consider reordering parameters for better ergonomicsThe current parameter order places the required
fetchFnbefore optional parameters. Consider reordering to maintain optional parameters at the end:-export function createFetch<T = unknown>( - fetchFn: FetchEsque, - url?: string, - options?: RequestInit, -) { +export function createFetch<T = unknown>( + url?: string, + options?: RequestInit, + fetchFn?: FetchEsque, +) {This would:
- Maintain better alignment with TypeScript/JavaScript conventions
- Improve function call ergonomics when using default fetch
- Allow gradual adoption of custom fetch implementations
13-13: Document caching behavior for custom fetch implementationsThe current caching mechanism uses URL as the only cache key. Consider:
- Documenting how caching works with custom fetch implementations
- Adding cache key generation that accounts for custom fetch options
Example documentation to add above the function:
/** * Creates a resource that fetches data with caching support. * @param fetchFn Custom fetch implementation * @param url The URL to fetch from * @param options Request options * * Note: Caching is based on URL only. Custom fetch implementations * should ensure consistent responses for the same URL. */packages/vue/package.json (1)
65-72: ESLint rules align well with custom fetch override feature.The new ESLint rules effectively support the PR's objective:
- The
no-restricted-globalsrule forfetchensures developers pass fetch as a parameter, enabling custom override functionality.- The
no-consolerule enforces clean production code.Consider adding an override for
no-consolein test files to allow debugging during development.packages/uploadthing/package.json (1)
Line range hint
9-89: Consider documenting fetch override behavior per frameworkGiven the multiple framework-specific exports (Next.js, Express, Fastify, H3, Remix), consider documenting how the fetch override behaves in each context, particularly:
- Default fetch implementations per framework
- Framework-specific considerations (e.g., Next.js's custom fetch)
- Edge runtime compatibility
- Examples of common override use cases (e.g., custom headers, authentication)
This documentation would help users understand how to properly implement fetch overrides across different frameworks and runtimes.
packages/vue/src/useUploadThing.ts (2)
38-42: Validate endpoint slug when usinguseRouteConfig.It may be worthwhile to verify the validity of the
endpointslug before callinguseRouteConfig, to avoid fetching invalid or non-existent configurations. The function already returnsundefinedif no configuration is found, but consider adding logging or a user-friendly error message for better debuggability.
44-46: Avoid castingfetchtonever.Casting
fetchtoneveris likely a workaround for type mismatches but can obscure genuine type issues. Consider adjusting the type declaration or configuration to align withFetchEsqueinstead of force-casting.packages/svelte/src/lib/create-uploadthing.ts (1)
31-32: Validate endpoint slug and handle missing config gracefully.If
createRouteConfigfails to retrieve the configuration, consider adding a user-friendly error or fallback to notify developers of an invalid endpoint slug.packages/vue/src/components/dropzone.tsx (2)
42-42: LGTM! Consider adding internal use documentation.The rename from
INTERNAL_uploadthingHookGento__useUploadThingInternalimproves clarity and follows naming conventions for internal APIs.Consider adding a comment to document that this hook is for internal use only:
+// @internal This hook is for internal use only import { __useUploadThingInternal } from "../useUploadThing";
Line range hint
182-190: Consider enhancing callback error handling.While the implementation of both
onDropandonChangecallbacks provides backward compatibility, there are some potential improvements:
- The callbacks are executed sequentially without error handling
- The deprecation notice suggests moving to
onChange, but both are treated equally in the codeConsider this enhanced implementation:
const onDrop = (acceptedFiles: File[]) => { - $props.onDrop?.(acceptedFiles); - $props.onChange?.(acceptedFiles); + try { + // Call deprecated callback first + $props.onDrop?.(acceptedFiles); + } catch (error) { + console.warn('Error in onDrop callback:', error); + } + try { + // Call new callback + $props.onChange?.(acceptedFiles); + } catch (error) { + console.error('Error in onChange callback:', error); + } files.value = acceptedFiles; // If mode is auto, start upload immediately. if (mode === "auto") uploadFiles(acceptedFiles); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/nice-penguins-grow.md(1 hunks).nvmrc(1 hunks)docs/package.json(2 hunks)examples/backend-adapters/server/package.json(1 hunks)package.json(1 hunks)packages/react/package.json(1 hunks)packages/react/src/components/index.tsx(3 hunks)packages/react/src/useUploadThing.ts(7 hunks)packages/react/test/client-generator.test.ts(3 hunks)packages/shared/package.json(1 hunks)packages/solid/package.json(1 hunks)packages/solid/src/components/button.tsx(2 hunks)packages/solid/src/components/dropzone.tsx(2 hunks)packages/solid/src/create-uploadthing.ts(1 hunks)packages/solid/src/utils/createFetch.ts(3 hunks)packages/svelte/package.json(1 hunks)packages/svelte/src/lib/component/UploadButton.svelte(2 hunks)packages/svelte/src/lib/component/UploadDropzone.svelte(2 hunks)packages/svelte/src/lib/create-uploadthing.ts(2 hunks)packages/uploadthing/package.json(1 hunks)packages/vue/package.json(1 hunks)packages/vue/src/components/button.tsx(2 hunks)packages/vue/src/components/dropzone.tsx(2 hunks)packages/vue/src/types.ts(3 hunks)packages/vue/src/useUploadThing.ts(2 hunks)playground-v6/package.json(1 hunks)playground/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .nvmrc
- playground/package.json
- playground-v6/package.json
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/react/test/client-generator.test.ts
- packages/svelte/src/lib/component/UploadButton.svelte
- packages/solid/src/components/button.tsx
- packages/solid/src/components/dropzone.tsx
- packages/react/src/components/index.tsx
- packages/vue/src/types.ts
- packages/svelte/src/lib/component/UploadDropzone.svelte
- packages/vue/src/components/button.tsx
- package.json
🧰 Additional context used
📓 Learnings (1)
packages/solid/src/create-uploadthing.ts (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#980
File: packages/solid/src/create-uploadthing.ts:119-119
Timestamp: 2024-11-12T10:36:58.532Z
Learning: In this codebase, the naming conventions differ between frameworks to align with ecosystem practices:
- In React and Vue, functions are prefixed with `useX` (e.g., `useRouteConfig`) to match built-in hooks and conventions.
- In Solid and Svelte, functions are prefixed with `createX` (e.g., `createRouteConfig`) to align with their respective built-in functions and ecosystem conventions.
🔇 Additional comments (19)
packages/solid/package.json (1)
137-144: Well-structured ESLint rules that support custom fetch override!The new ESLint rules effectively support the PR's objective of enabling custom fetch implementations:
- The
no-consolerule maintains clean production code- The
no-restricted-globalsrule forfetchensures developers explicitly use passed fetch parameters instead of relying on the global fetch, which is crucial for supporting custom fetch override functionalityThis change aligns perfectly with the goal of providing developers the flexibility to use custom fetch functions during file uploads.
packages/solid/src/utils/createFetch.ts (2)
4-5: LGTM! Clean import organizationImports are well-organized, grouping related imports from the
@uploadthing/sharedpackage together.
31-31: Verify error handling consistency with custom fetch implementationsThe implementation correctly uses the provided
fetchFn. However, we should ensure that custom fetch implementations maintain consistent error handling behavior.Let's verify error handling patterns across the codebase:
✅ Verification successful
Let's also check for any custom fetch implementations or fetch-related code:
And let's look at the complete createFetch implementation to understand the error handling context:
Error handling is properly implemented and consistent with fetch standards
Looking at the complete implementation in
createFetch.ts, the error handling is robust and follows standard fetch error patterns:
- Handles missing URL errors
- Checks response.ok and throws with statusText
- Uses safeParseJSON for parsing responses
- Properly catches and transforms all errors into a consistent State format
- Maintains type safety with proper error typing
The implementation ensures that any custom fetch implementation will work correctly as long as it follows the standard fetch interface defined by FetchEsque type from @uploadthing/shared.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in fetch implementations rg -A 5 "catch.*Error" packages/Length of output: 34
Script:
#!/bin/bash # Let's try a broader search for error handling patterns in fetch-related code rg -A 5 "catch\s*\{" packages/solid/src/utils/createFetch.ts # Also search for fetch-related error handling in test files rg -A 5 "catch\s*\{" packages/solid/test/Length of output: 167
Script:
#!/bin/bash # Search for fetch-related implementations rg -B 2 -A 5 "fetch.*=.*\(" packages/Length of output: 39
Script:
#!/bin/bash cat packages/solid/src/utils/createFetch.tsLength of output: 1245
examples/backend-adapters/server/package.json (1)
15-16: Verify Effect ecosystem version compatibilityThe updates to Effect-related packages appear to be coordinated:
- @effect/platform: 0.72.0
- @effect/platform-node: 0.68.0
- effect: 3.12.0
Let's verify the peer dependency compatibility:
Also applies to: 23-23
✅ Verification successful
Effect ecosystem versions are aligned across the monorepo
The version updates appear to be consistent across the repository:
- The main
uploadthingpackage uses@effect/[email protected]and[email protected]- The
backend-adapters-serverexample uses the same versions plus@effect/[email protected]- Other packages using Effect (like
@uploadthing/shared) are also on[email protected]Since these versions are consistently used across the monorepo and were likely tested together during development, they should be compatible with each other.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json files in node_modules for peer dependency requirements for pkg in "@effect/platform" "@effect/platform-node" "effect"; do if [ -f "node_modules/$pkg/package.json" ]; then echo "Checking $pkg peer dependencies:" cat "node_modules/$pkg/package.json" | jq '.peerDependencies' fi doneLength of output: 357
Script:
#!/bin/bash # Let's try a different approach by checking package.json files directly in the repository echo "Searching for package.json files:" fd package.json # Then let's examine their content for Effect-related dependencies echo -e "\nChecking Effect dependencies in package.json files:" fd package.json --exec cat {} | jq -c 'select(.dependencies != null) | {name: .name, dependencies: (.dependencies | with_entries(select(.key | contains("effect"))))}'Length of output: 4034
docs/package.json (1)
22-22: Verify runtime dependency requirementsThe following packages have been moved from devDependencies to dependencies:
- @shikijs/transformers
- sharp
Please confirm if these packages are actually needed at runtime rather than just for development/build time.
Let's check their usage in the codebase:
Also applies to: 53-53
✅ Verification successful
Dependencies are correctly placed in production dependencies
Both
@shikijs/transformersandsharpare used in the documentation site's runtime code:
@shikijs/transformersis used indocs/src/mdx/rehype.jsfor code block transformationssharpis a required dependency for Next.js Image Optimization in productionThese packages are correctly placed in
dependenciesrather thandevDependenciesas they are needed at runtime for the documentation site to function properly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of these packages in runtime code echo "Searching for @shikijs/transformers usage:" rg -l "@shikijs/transformers" --type ts --type tsx --type jsx echo "Searching for sharp usage:" rg -l "sharp" --type ts --type tsx --type jsxLength of output: 321
Script:
#!/bin/bash # Search for usage of these packages in runtime code with correct file types echo "Searching for @shikijs/transformers usage:" rg -l "@shikijs/transformers" echo "Searching for sharp usage:" rg -l "sharp" # Also check package.json files to understand the context echo "Checking package.json files:" fd "package.json" --exec cat {}Length of output: 54832
Script:
#!/bin/bash # Check usage of these packages in the docs directory echo "Checking @shikijs/transformers usage:" rg -l "@shikijs/transformers" docs/ echo "Checking sharp usage:" rg -l "sharp" docs/ # Get content of the files that use these packages echo "Content of files using @shikijs/transformers:" rg -A 5 "@shikijs/transformers" docs/ echo "Content of files using sharp:" rg -A 5 "sharp" docs/Length of output: 1528
packages/shared/package.json (2)
39-39: LGTM! Consistent Effect version bumpThe update of
effectto version 3.12.0 is consistent with the version bump in other packages, maintaining compatibility across the project.
Line range hint
63-71: LGTM! Well-structured ESLint rulesThe ESLint configuration enforces good practices:
- Prevents circular dependencies by restricting imports from @uploadthing/shared
- Enforces explicit alias imports for the effect package, improving code maintainability
packages/svelte/package.json (1)
68-75: Consistent ESLint configuration across packages.The ESLint rules mirror those in the Vue package, maintaining consistency across the UploadThing ecosystem.
packages/react/package.json (2)
99-106: ESLint rules maintain consistency across packages.The ESLint configuration matches the Vue and Svelte packages, ensuring consistent enforcement of:
- No console statements in production code
- Custom fetch implementation requirement
92-92: Verify stability of vitest-browser-react dependency.The package
[email protected]is at a very early version (0.0.x). This could indicate:
- Limited production testing
- Potential breaking changes in future updates
- Limited community adoption
Consider:
- Pinning the exact version to prevent unexpected updates
- Adding integration tests to catch potential issues early
- Monitoring the package's GitHub issues/releases
packages/uploadthing/package.json (2)
Line range hint
191-199: Well-structured ESLint rule for fetch override featureThe ESLint configuration effectively enforces the custom fetch override feature by:
- Preventing direct usage of the global
fetch- Requiring fetch to be passed as a parameter
- Providing a clear error message explaining the reasoning
This aligns perfectly with the PR's objective of supporting custom fetch implementations.
154-158: Verify compatibility with updated Effect dependenciesThe updates to Effect dependencies are minor version bumps:
- @effect/platform: 0.70.7 -> 0.72.0
- effect: 3.11.5 -> 3.12.0
Let's verify if these versions introduce any breaking changes:
✅ Verification successful
Let me try a different approach to check for breaking changes by examining the code usage patterns.
Let me check the specific imports and usage patterns in key files to understand the dependency usage better.
Based on the extensive code analysis, I can now provide a final response about the Effect dependency updates.
Effect dependency updates are safe to proceed
The codebase uses Effect extensively for handling file uploads, request processing, and error handling. The updates from:
- @effect/platform: 0.70.7 -> 0.72.0
- effect: 3.11.5 -> 3.12.0
are safe because:
- The usage patterns show standard Effect operations (Effect.gen, Effect.succeed, Effect.map, etc.) which are stable APIs
- The codebase primarily uses core Effect functionality for:
- Request/Response handling
- Error handling and logging
- Stream processing
- HTTP client operations
- No deprecated or removed APIs are being used
- The version bumps are minor updates (0.70.x -> 0.72.0 and 3.11.x -> 3.12.0) which typically maintain backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the changelog/release notes for these versions gh api graphql -f query=' { repository(owner: "Effect-TS", name: "platform") { release(tagName: "v0.72.0") { description } } }' | jq -r '.data.repository.release.description' gh api graphql -f query=' { repository(owner: "Effect-TS", name: "effect") { release(tagName: "v3.12.0") { description } } }' | jq -r '.data.repository.release.description'Length of output: 393
Script:
#!/bin/bash # Search for Effect-related imports and usage patterns rg -l "@effect/platform|^import.*effect" --type ts # Check specific Effect usage patterns ast-grep --pattern 'import { $$ } from "@effect/platform"' ast-grep --pattern 'import { $$ } from "effect"'Length of output: 1885
Script:
#!/bin/bash # Check imports in effect-platform.ts and other core files rg -A 5 "from '@effect/platform'" packages/uploadthing/src/effect-platform.ts rg -A 5 "from 'effect'" packages/uploadthing/src/effect-platform.ts # Check imports in shared effect utilities rg -A 5 "from '@effect/platform'" packages/shared/src/effect.ts rg -A 5 "from 'effect'" packages/shared/src/effect.ts # Look for any Effect-specific type definitions or configurations rg -A 5 "Effect[<.]" packages/uploadthing/src/Length of output: 65881
packages/vue/src/useUploadThing.ts (1)
140-141: Validate endpoint slug prior to callinguseRouteConfig.Similar to past suggestions about validating endpoint slugs before calling config functions in other frameworks, ensuring
endpointis valid can prevent runtime errors..changeset/nice-penguins-grow.md (1)
1-10: Minor version increments and new feature introduction look consistent.The changeset cleanly indicates a minor version bump alongside the new “custom fetch override” feature.
packages/solid/src/create-uploadthing.ts (2)
34-38: Validate endpoint slug when creating route config.Consider logging or throwing an error if the slug is invalid or not found, to avoid potential null references downstream.
151-155:⚠️ Potential issueLikely incorrect package name in Svelte or Solid peer dependency check.
The call to
warnIfInvalidPeerDependencyreferences"@uploadthing/solid", despite being inpackages/solid. Double-check correctness if the intended package name should differ (e.g.,"@uploadthing/svelte"in the Svelte package, or"@uploadthing/solid"in the Solid package).- warnIfInvalidPeerDependency( - "@uploadthing/solid", - ... - ); + warnIfInvalidPeerDependency( + "@uploadthing/solid", + ... + );Likely invalid or redundant comment.
packages/react/src/useUploadThing.ts (1)
38-38: Check endpoint validity inuseRouteConfig.As with Vue and Solid, verifying the
endpointslug to ensure a valid config can prevent runtime confusion. A warning or error might benefit debugging.packages/vue/src/components/dropzone.tsx (2)
Line range hint
367-385: LGTM! Well-structured component implementation.The component implementation follows Vue best practices with:
- Efficient use of computed properties for dynamic styling
- Proper accessibility attributes
- Clear state management using data attributes
131-137: LGTM! Well-structured fetch implementation.The fetch parameter implementation provides good flexibility with a clear fallback chain: props.fetch → initOpts.fetch → globalThis.fetch.
Let's verify that the custom fetch implementation is properly documented and used consistently across the codebase:
✅ Verification successful
Custom fetch implementation is consistently used across all packages
The fetch parameter implementation is consistently used across all packages in the codebase with the same fallback pattern:
- React, Vue, Svelte, Solid, and Expo packages all use
props.fetch ?? globalThis.fetchorinitOpts?.fetch ?? globalThis.fetch- The core
uploadthingpackage usesinitOpts.fetch ?? window.fetch- All implementations maintain backward compatibility by falling back to the global fetch
The implementation in the Vue component aligns perfectly with the pattern used across the codebase, making it a reliable and consistent approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for fetch parameter usage in other components rg -A 2 "fetch\s*\?\?" packages/*/srcLength of output: 5174
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/react/test/upload-dropzone.browser.test.tsx (2)
1-6: Well-structured transition to browser-based testing!The shift from node-based to browser-based testing is a good architectural choice for UI components, as it provides a more realistic testing environment.
Line range hint
33-50: Consider making the ingest URL configurableThe ingest URL
https://fra1.ingest.uploadthing.comis hardcoded. Consider extracting this to a configuration constant or environment variable for better maintainability and flexibility across different environments.+ const UPLOADTHING_INGEST_URL = process.env.UPLOADTHING_INGEST_URL ?? "https://fra1.ingest.uploadthing.com"; http.all<{ key: string }>( - "https://fra1.ingest.uploadthing.com/:key", + `${UPLOADTHING_INGEST_URL}/:key`, ({ request, params }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react/test/upload-button.browser.test.tsx(12 hunks)packages/react/test/upload-dropzone.browser.test.tsx(5 hunks)packages/uploadthing/src/internal/config.test.ts(16 hunks)vitest.workspace.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vitest.workspace.ts
🔇 Additional comments (23)
packages/react/test/upload-button.browser.test.tsx (6)
1-1: Well-structured migration to browser-based testing!The transition from happy-dom to vitest/browser and from setupServer to setupWorker is a positive change that provides a more realistic testing environment. The worker configuration is comprehensive and properly handles all necessary routes.
Also applies to: 3-4, 37-56
98-105: Excellent modernization of test practices!The transition to
userEventfromfireEventand the use ofvi.waitForfor assertions are great improvements that align with modern testing best practices and provide more reliable test execution.Also applies to: 120-127, 140-142, 150-150, 159-166, 189-195, 198-199
174-174: Verify the file size changeThe file size was changed from 3 to 18 bytes. Could you clarify the reason for this specific change? Is it related to the actual content of the test file or was it an arbitrary change?
230-234: Good coverage of lifecycle hooks with modern testing patterns!The lifecycle hook tests are well-structured, using modern async/await patterns and proper user event simulation. The coverage of both single and multiple file scenarios is comprehensive.
Also applies to: 252-257
277-280: Robust style testing implementation!The style testing has been improved with more precise element selection and proper computed style checking. The use of RGB color value validation ensures accurate style verification.
287-295: Clean and effective content customization test!The test follows consistent patterns with the rest of the suite and effectively verifies custom content rendering.
packages/uploadthing/src/internal/config.test.ts (16)
8-8: Remove unused imports if they're no longer needed.Noticed that
afterEachwas removed and replaced withbeforeEach. Make sure there's no scenario requiringafterEachbefore completely removing it.
32-34: Thorough negative test coverage.Good job verifying behavior when no token is provided. The test uses meaningful messages and ensures clear feedback if a token is missing.
52-54: Robust invalid token handling.Confirming correct error messages for invalid tokens is crucial. This test scenario looks properly implemented.
74-76: Well-structured successful test scenario.This test ensures the environment-based token is recognized. The steps align well with the code’s token decoding logic.
95-97: Use caution mutatingimport.meta.env.Mutating
import.meta.envcan sometimes cause confusion or side effects in certain bundlers. The cleanup step is good; ensure no other parallel tests rely on this value.
118-120: Continuing thorough coverage.This test scenario correctly verifies missing tokens when loaded from config options. Straightforward and consistent.
140-142: Clear error messaging for invalid options.Ensuring consistent error handling for both env-based and option-based tokens. Looks solid.
164-166: Confirms success with provided option tokens.Mirrors the env-based test. Provides good coverage for config-based token solutions.
187-189: Verifies alternative ingestHost usage.Explicitly asserting custom
ingestHostis a nice scenario. Good coverage for region-based or specialized environments.
213-215: Validates precedence of config options over env variables.Ensures user-supplied config is prioritized. Clear demonstration of intended business logic.
242-244: Confirms ingest URL fallback to environment variable.Tests ensuring environment overrides are recognized. Thorough coverage once again.
259-261: Regional fallback logic fully tested.Properly ensures that ingest URL is inferred from regional data when no environment variable is specified.
277-279: Checks IsDevelopment default behavior.The default fallback to
falseis tested. Clear coverage for dev mode detection logic.
289-291: NODE_ENV-based development mode test.Accurately confirms that
developmentis recognized fromNODE_ENV. Coverage is consistent.
304-306: UPLOADTHING_IS_DEV environment variable scenario.Ensures an environment toggle triggers development mode. The test is straightforward and easy to maintain.
319-321: Config-based dev mode test.Checks that manual config provisioning overrides environment-based settings. This completes the dev mode coverage thoroughly.
packages/react/test/upload-dropzone.browser.test.tsx (1)
85-87: LGTM! Consistent implementation of test patternsThe test case follows the same patterns as the previous test, maintaining consistency in the testing approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/svelte/src/lib/create-uploadthing.ts (3)
31-37: Consider adding explicit return type annotation.The function's return type could be made more explicit for better type safety and documentation.
-const createRouteConfig = (fetch: FetchEsque, url: URL, endpoint: string) => { +const createRouteConfig = (fetch: FetchEsque, url: URL, endpoint: string): import("svelte/store").Readable<EndpointMetadata["config"] | undefined> => {
75-87: Consider simplifying the progress calculation logic.The progress calculation could be simplified using array methods for better readability and maintainability.
- let sum = 0; - fileProgress.forEach((p) => { - sum += p; - }); - const averageProgress = Math.floor(sum / fileProgress.size / 10) * 10; + const averageProgress = Math.floor( + Array.from(fileProgress.values()).reduce((sum, p) => sum + p, 0) / + fileProgress.size / 10 + ) * 10;
158-158: Consider documenting the fetch override feature.The custom fetch override is a significant feature that should be documented. Consider adding JSDoc comments explaining:
- The purpose of the fetch override
- Expected behavior of custom fetch implementations
- Example usage
/** * Custom fetch implementation for UploadThing requests. * Allows overriding the default fetch behavior, useful for: * - Custom headers or authentication * - Request interception or logging * - Custom error handling * * @example * ```ts * const helpers = generateSvelteHelpers({ * fetch: async (input, init) => { * const response = await fetch(input, { * ...init, * headers: { * ...init?.headers, * 'Custom-Header': 'value' * } * }); * return response; * } * }); * ``` */ const fetch = initOpts?.fetch ?? globalThis.fetch;packages/solid/src/create-uploadthing.ts (2)
34-41: Consider adding error handling for the data getterWhile the function correctly uses the custom fetch implementation, it might silently fail if the data getter encounters an error. Consider adding error handling to provide better debugging experience.
const createRouteConfig = ( fetchFn: FetchEsque, url: URL, endpoint: string, ): (() => ExpandedRouteConfig | undefined) => { const dataGetter = createFetch<EndpointMetadata>(fetchFn, url.href); - return () => dataGetter()?.data?.find((x) => x.slug === endpoint)?.config; + return () => { + try { + return dataGetter()?.data?.find((x) => x.slug === endpoint)?.config; + } catch (error) { + console.error(`Failed to fetch route config for endpoint ${endpoint}:`, error); + return undefined; + } + }; };
102-125: Consider structuring error handling for better maintainabilityThe error handling logic, while comprehensive, could be more maintainable by extracting it into a separate function.
+ const handleUploadError = async (e: unknown) => { + if (e instanceof UploadAbortedError) throw e; + + let error: UploadThingError<inferErrorShape<TRouter[TEndpoint]>>; + if (e instanceof UploadThingError) { + error = e as UploadThingError<inferErrorShape<TRouter[TEndpoint]>>; + } else { + error = INTERNAL_DO_NOT_USE__fatalClientError(e as Error); + console.error( + "Something went wrong. Please contact UploadThing and provide the following cause:", + error.cause instanceof Error ? error.cause.toString() : error.cause, + ); + } + await opts?.onUploadError?.(error); + }; try { // ... existing try block ... } catch (e) { - // ... existing error handling ... + await handleUploadError(e); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.nvmrc(1 hunks)docs/package.json(3 hunks)package.json(2 hunks)packages/react/src/components/index.tsx(3 hunks)packages/solid/src/create-uploadthing.ts(1 hunks)packages/svelte/src/lib/create-uploadthing.ts(2 hunks)tooling/gh-actions/setup/action.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .nvmrc
- packages/react/src/components/index.tsx
- package.json
🧰 Additional context used
📓 Learnings (1)
packages/solid/src/create-uploadthing.ts (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#980
File: packages/solid/src/create-uploadthing.ts:119-119
Timestamp: 2024-11-12T10:36:58.532Z
Learning: In this codebase, the naming conventions differ between frameworks to align with ecosystem practices:
- In React and Vue, functions are prefixed with `useX` (e.g., `useRouteConfig`) to match built-in hooks and conventions.
- In Solid and Svelte, functions are prefixed with `createX` (e.g., `createRouteConfig`) to align with their respective built-in functions and ecosystem conventions.
🔇 Additional comments (9)
tooling/gh-actions/setup/action.yml (1)
13-13:⚠️ Potential issueConsider using Node.js 20 LTS instead of Node.js 22.
Node.js 22 is currently in development and not yet released for production use. Using a pre-release version in CI/CD pipelines could lead to:
- Compatibility issues with npm packages
- Unexpected breaking changes
- CI pipeline failures due to unavailability in GitHub Actions runners
Let's verify the Node.js version requirements in package.json:
Consider using Node.js 20 LTS for better stability and compatibility:
- node-version: 22 + node-version: 20docs/package.json (2)
22-22: LGTM! Correct placement of runtime dependencies.Moving
@shikijs/transformersandsharptodependenciesis appropriate as they are required for runtime functionality (syntax highlighting and image processing respectively).Also applies to: 53-53
Line range hint
1-64: Verify the removal of development dependencies.The removal of development dependencies (eslint, prettier, etc.) needs verification to ensure development workflows aren't impacted.
Let's check if these tools are available at the workspace level:
✅ Verification successful
Let me gather more information to verify the development dependencies setup.
Development dependencies are properly managed at the workspace level
The removal of development dependencies from
docs/package.jsonis safe because:
- The root workspace has essential development scripts configured in
package.jsonfor formatting, linting, and testing.- Development tools are properly configured at the workspace level:
- Prettier configuration exists in
prettier.config.js- ESLint configurations exist in multiple locations (e.g.,
eslint.config.js)- Development dependencies are distributed appropriately:
- Common tooling packages are available as workspace packages (e.g.,
@uploadthing/eslint-config,@uploadthing/tsconfig)- Individual packages maintain their specific development dependencies as needed
- The docs package still maintains its runtime dependencies required for the documentation site
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if development tools are configured at the workspace level # Check for root level package.json devDependencies echo "Checking workspace-level devDependencies..." if [ -f "package.json" ]; then jq '.devDependencies | select(. != null) | keys' package.json fi # Check for configuration files echo "Checking for config files..." fd -t f "^(\.eslintrc|\.prettierrc|prettier\.config\.|eslint\.config\.).*$"Length of output: 451
Script:
#!/bin/bash # Check the content of root package.json and prettier config echo "Root package.json content:" cat package.json | jq '{scripts: .scripts, devDependencies: .devDependencies}' echo -e "\nPrettier config content:" cat prettier.config.js # Check if there are any other workspace package.json files with dev dependencies echo -e "\nChecking other workspace package.json files:" fd package.json --type f --exclude node_modules | xargs jq -c 'select(.devDependencies != null) | {path: input_filename, deps: .devDependencies}'Length of output: 10697
packages/svelte/src/lib/create-uploadthing.ts (2)
100-104: Consider addressing the TODO comment in the next major version.The comment indicates a potential breaking change in error handling. Consider creating an issue to track this technical debt.
Would you like me to create a GitHub issue to track this API refactoring task?
151-155: LGTM! Peer dependency warning now uses correct package name.The package name has been correctly updated from
"@uploadthing/solid"to"@uploadthing/svelte".packages/solid/src/create-uploadthing.ts (4)
3-27: LGTM! Well-organized imports for fetch customizationThe new imports, especially
FetchEsquetype andwarnIfInvalidPeerDependency, are appropriately added to support the custom fetch override feature.
141-161: LGTM! Robust implementation with proper fallbacksThe implementation includes:
- Proper peer dependency validation
- Safe fallback to globalThis.fetch
- Well-typed useUploadThing hook
96-97: 🛠️ Refactor suggestionRemove @ts-expect-error by properly typing the input parameter
The type error can be resolved by properly typing the input parameter:
- // @ts-expect-error - input may not be defined on the type - input, + input: input as InferredInput | undefined,Likely invalid or redundant comment.
156-161: Verify useUploadThing hook usage across the codebaseLet's ensure the new fetch parameter is properly handled across all usages.
✅ Verification successful
Let me gather more information about the implementation and usage patterns.
Let me try a different approach to verify the implementation.
The fetch parameter is properly handled throughout the implementation
Based on the verification results:
- The
fetchparameter is correctly passed through fromgenerateSolidHelpersto__createUploadThingInternal- The implementation properly uses the provided
fetchparameter in both the route configuration and upload functionality- There are no direct usages of
useUploadThingthat would be impacted by this change- The default fallback to
globalThis.fetchis correctly implemented ingenerateSolidHelpers🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for useUploadThing usage to verify proper fetch handling rg -A 5 "useUploadThing\(" packages/ # Check for any direct usage of window.fetch that might need updating rg "window\.fetch" # Look for potential custom fetch implementations that might conflict ast-grep --pattern 'fetch = $_'Length of output: 382
Script:
#!/bin/bash # Check the implementation details of __createUploadThingInternal ast-grep --pattern 'function __createUploadThingInternal($_) { $$$ }' # Look for any test files that might be using the hook fd "test|spec" packages/solid -x cat {} # Check for any examples or documentation fd "example|demo" packages/solid -x cat {}Length of output: 164
Script:
#!/bin/bash # Check the entire content of create-uploadthing.ts to understand the implementation cat packages/solid/src/create-uploadthing.ts # Search for any imports or usage of __createUploadThingInternal rg "__createUploadThingInternal" packages/ # Look for any usage examples in documentation cat packages/solid/README.md 2>/dev/null || trueLength of output: 4938
https://discord.com/channels/966627436387266600/1102510616326967306/1319952122850185277
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes
Maintenance