feat: forward request context to onUploadComplete and onUploadError
#1045
Conversation
🦋 Changeset detectedLatest commit: 9f3d8d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
|
WalkthroughThe changes introduce a new feature that forwards the request context to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
juliusmarminge
changed the title
onUploadComplete and onUploadError
More templates
commit: |
📦 Bundle size comparison
|
juliusmarminge
force-pushed
the
forward-request-args-to-cb
branch
from
17085b3 to
55df5bf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
packages/uploadthing/src/express.ts (1)
20-24: LGTM! Good architectural improvement.The renaming from
MiddlewareArgstoAdapterArgsbetter reflects the type's role in the adapter pattern and aligns with the codebase's architectural evolution.packages/uploadthing/src/effect-platform.ts (1)
Line range hint
16-20: Consider removing or updating the unusedMiddlewareArgstype.The local
MiddlewareArgstype appears to be unused since the codebase has moved to usingAdapterArguments. This could lead to confusion about which type should be used.Consider either:
- Removing this type if it's no longer needed
- Adding documentation to clarify its relationship with
AdapterArguments- Renaming it to match the new naming convention
-type MiddlewareArgs = { - req: HttpServerRequest.HttpServerRequest; - res: undefined; - event: undefined; -};packages/uploadthing/src/internal/upload-builder.ts (2)
10-14: LGTM! Good architectural improvement.The rename from
MiddlewareArgstoAdapterFnArgsreflects a better architectural pattern, making it clearer that these arguments can be used beyond just middleware functions.
Line range hint
95-115: LGTM! Consider updating documentation.The type changes are consistently applied in the public interface. Since this is a significant architectural change, consider:
- Updating the documentation to reflect the new adapter pattern
- Adding examples of how to access request context in
onUploadCompleteandonUploadErrorpackages/uploadthing/src/internal/handler.ts (1)
50-52: Consider adding type tests for adapter argumentsTo ensure type safety of the forwarded context, consider adding type tests that verify the shape of the adapter arguments being passed to various handlers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
.changeset/warm-moons-count.md(1 hunks)examples/backend-adapters/server/src/router.ts(1 hunks)packages/uploadthing/src/effect-platform.ts(2 hunks)packages/uploadthing/src/express.ts(1 hunks)packages/uploadthing/src/fastify.ts(1 hunks)packages/uploadthing/src/h3.ts(1 hunks)packages/uploadthing/src/internal/handler.ts(9 hunks)packages/uploadthing/src/internal/types.ts(7 hunks)packages/uploadthing/src/internal/upload-builder.ts(4 hunks)packages/uploadthing/src/next-legacy.ts(1 hunks)packages/uploadthing/src/next.ts(1 hunks)packages/uploadthing/src/remix.ts(1 hunks)packages/uploadthing/src/server.ts(1 hunks)packages/uploadthing/test/request-handler.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/uploadthing/src/internal/types.ts
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (31)
.changeset/warm-moons-count.md (1)
1-5: LGTM! The changeset correctly documents the feature.
The minor version bump is appropriate for this new feature as it adds functionality in a backwards-compatible manner. The description clearly communicates the enhancement of forwarding request context to the callback functions.
packages/uploadthing/src/next.ts (2)
14-18: LGTM! Type rename improves clarity.
The rename from MiddlewareArgs to AdapterArgs better reflects its purpose as an adapter-specific configuration type. The explicit undefined types for res and event clearly indicate Next.js adapter specifics.
21-22: LGTM! Type update maintains consistency.
The update to use AdapterArgs maintains type consistency while preserving the ability to access request context, which aligns with the PR objective of forwarding context to upload handlers.
Let's verify the type usage across the codebase:
✅ Verification successful
Type consistency verified across adapters
The update to use AdapterArgs is consistent with the type usage across all platform adapters. Each adapter (next.ts, remix.ts, express.ts, fastify.ts, h3.ts, next-legacy.ts) properly defines its own AdapterArgs type with the appropriate request/response/event types for that platform. The only remaining MiddlewareArgs reference is in effect-platform.ts which is a separate implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of AdapterArgs type
# Expected: No remaining references to MiddlewareArgs
# Test 1: Check for any remaining references to MiddlewareArgs
rg "MiddlewareArgs"
# Test 2: Verify AdapterArgs is used consistently in adapter files
rg "AdapterArgs" "packages/uploadthing/src/*"
Length of output: 299
Script:
#!/bin/bash
# Let's verify more thoroughly with correct paths and patterns
# Check for AdapterArgs usage
rg "AdapterArgs"
# Check the specific file content to understand the context better
cat "packages/uploadthing/src/next.ts"
# Look for any type definitions related to request context
ast-grep --pattern 'type $_ = {
$$$
}'
Length of output: 28469
packages/uploadthing/src/remix.ts (2)
14-17: LGTM! Type renaming improves clarity.
The renaming from MiddlewareArgs to AdapterArgs better reflects its role in the adapter pattern while maintaining the same structure. The explicit undefined fields for req and res make sense in the Remix context where we rely on Remix's own request/response abstractions via ActionFunctionArgs.
19-22: Verify the impact of type changes on existing implementations.
The update to use AdapterArgs in the builder creation looks good and maintains type safety. However, we should verify that existing implementations aren't affected by this type change.
✅ Verification successful
No impact on existing implementations - type change is framework-specific
The type change from MiddlewareArgs to AdapterArgs in remix.ts is correct and safe. Each framework-specific adapter (express, remix, next, h3, etc.) defines its own AdapterArgs type tailored to its context. The MiddlewareArgs found in effect-platform.ts is a separate implementation for the Effect platform adapter and is not affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing implementations that might be affected by the type change
# Look for files importing from this module
rg -l "from ['\"]@uploadthing/remix['\"]"
# Look for direct usage of the old MiddlewareArgs type
rg "MiddlewareArgs"
Length of output: 234
Script:
#!/bin/bash
# Let's check the content of effect-platform.ts to understand the MiddlewareArgs usage
cat packages/uploadthing/src/effect-platform.ts
# Also check if there are any imports or references to AdapterArgs
rg "AdapterArgs" -A 2
Length of output: 5263
packages/uploadthing/src/h3.ts (2)
15-19: LGTM! Clear and descriptive type renaming.
The rename from MiddlewareArgs to AdapterArgs better reflects its purpose as adapter-specific arguments, improving code clarity while maintaining the same structure.
21-23: LGTM! Consistent type parameter update.
The generic type parameter update in createBuilder<AdapterArgs, TErrorShape> maintains consistency with the type renaming.
packages/uploadthing/src/server.ts (1)
17-21: Type renaming improves semantic clarity.
The renaming from MiddlewareArgs to AdapterArgs better reflects its purpose as adapter-specific context. The type structure correctly maintains the request context while explicitly marking res and event as undefined for this adapter.
packages/uploadthing/src/fastify.ts (2)
15-19: LGTM! Semantically accurate type renaming.
The renaming from MiddlewareArgs to AdapterArgs better reflects its role in the adapter pattern, while maintaining the same structure for request context access.
21-23: LGTM! Consistent type parameter update.
The type parameter change in createUploadthing correctly reflects the renamed type while preserving the error handling capabilities through TErrorShape.
Let's verify the consistency of this change across other adapter files:
packages/uploadthing/src/next-legacy.ts (1)
15-19: LGTM! Improved type naming.
The rename from MiddlewareArgs to AdapterArgs better reflects its purpose as adapter-specific arguments, improving code clarity.
packages/uploadthing/src/express.ts (1)
28-28: LGTM! Verify consistent type usage across adapters.
The type parameter update enables request context forwarding while maintaining type safety.
Let's verify consistent type usage across other adapter files:
packages/uploadthing/src/effect-platform.ts (2)
8-8: LGTM: Import renamed for consistency.
The import change from MiddlewareArguments to AdapterArguments aligns with the broader refactoring effort across the codebase.
55-55: LGTM: Type reference updated consistently.
The usage of AdapterArguments is consistent with the import changes.
examples/backend-adapters/server/src/router.ts (1)
64-65:
Implementation appears incomplete for request context forwarding.
Based on the PR objectives, this change should forward the request context to the onUploadComplete callback. However, the current implementation only shows the destructured metadata and file parameters, without exposing the request context.
Let's verify if the request context is being properly forwarded:
Could you please clarify:
- How is the request context being forwarded to this callback?
- Should the callback signature include a
ctxparameter?
packages/uploadthing/src/internal/upload-builder.ts (2)
Line range hint 18-27: LGTM! Type parameters properly updated.
The change from TMiddlewareArgs to TAdapterFnArgs is consistently applied in both the function signature and return type structure, maintaining type safety while enabling request context forwarding.
Line range hint 32-93: Verify backward compatibility of the builder methods.
While the type changes look good, we should verify that existing code using these builder methods continues to work as expected.
✅ Verification successful
Builder methods maintain backward compatibility
Based on the extensive usage patterns found across test files, examples, and documentation, the builder methods (input, middleware, onUploadComplete, onUploadError) maintain their existing behavior and signatures while supporting the new types. The implementation changes are type-level only and do not affect runtime behavior.
Key evidence:
- Test files demonstrate consistent usage patterns across different scenarios
- Documentation and examples show unchanged method signatures
- No breaking changes in method implementations, only type enhancements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage of builder methods to verify compatibility
rg -A 2 '\.(input|middleware|onUploadComplete|onUploadError)\(' --type typescript
Length of output: 118
Script:
#!/bin/bash
# Search for existing usage of builder methods using ripgrep without file type filter
rg -A 2 '\.(input|middleware|onUploadComplete|onUploadError)\('
Length of output: 50252
packages/uploadthing/src/internal/types.ts (7)
Line range hint 51-55: LGTM: Appropriate rename to AdapterFnArgs
The rename from MiddlewareFnArgs to AdapterFnArgs better reflects its role as a framework adapter type, while maintaining the same robust generic structure.
64-64: LGTM: Consistent rename in AnyParams interface
The property rename maintains consistency with the AdapterFnArgs type change.
73-73: LGTM: Appropriate type constraint update
The generic constraint correctly uses the renamed AdapterFnArgs type.
81-90: LGTM: Successfully forwarded request context to onUploadComplete
The type now correctly includes the request context via TArgs while maintaining the existing metadata and file information structure.
Note: The static analysis warning about void in the union type can be safely ignored as void is semantically correct here to represent "no return value".
🧰 Tools
🪛 Biome
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
92-97: LGTM: Successfully forwarded request context to onUploadError
The type now correctly includes the request context via TArgs while maintaining the existing error handling structure.
108-108: LGTM: Consistent updates throughout UploadBuilder interface
All properties and method signatures have been correctly updated to use _adapterFnArgs and forward the request context appropriately.
Also applies to: 118-118, 125-125, 132-132, 138-138, 140-140, 150-151
178-178: LGTM: Completed request context forwarding in FileRoute interface
The interface correctly includes request context in both onUploadError and onUploadComplete signatures, completing the type system updates.
Also applies to: 180-180
packages/uploadthing/test/request-handler.test.ts (2)
429-437: LGTM! Well-structured request setup.
The separation of request construction improves test readability and allows for proper verification of the request context forwarding feature.
443-445: Consider documenting adapter-specific parameters.
The test verifies that event and res are undefined for the default adapter, while the request context is properly forwarded. Consider adding a comment explaining that these parameters are adapter-specific and may be defined when using specific backend adapters (e.g., Express, Fastify).
Let's verify the adapter-specific parameters in other files:
packages/uploadthing/src/internal/handler.ts (5)
16-16: LGTM! Consistent renaming to adapter pattern
The renaming from MiddlewareArguments to AdapterArguments and related type changes align well with the adapter pattern. The more descriptive Schema import name also improves code readability.
Also applies to: 48-48, 50-52
55-57: LGTM! Handler signature updated consistently
The parameter rename to makeAdapterArgs maintains consistency with the adapter pattern changes.
Line range hint 257-267: LGTM! Error handler now includes request context
The implementation correctly forwards the adapter arguments to onUploadError, fulfilling the PR's objective while maintaining robust error handling.
314-317: LGTM! Callback handler properly forwards context
The implementation successfully forwards adapter arguments to onUploadComplete while maintaining proper schema validation and callback data handling.
Also applies to: 329-336
394-400: LGTM! Middleware properly receives context
The implementation consistently forwards adapter arguments to the middleware function, maintaining the pattern established in other handlers.
currently you can only access the request in
middleware. This adds so you can also access it inonUploadCompleteandonUploadError. This is especially important in e.g. workers where your environment variables and bindings are on the request contextSummary by CodeRabbit
Release Notes
New Features
onUploadCompleteandonUploadErrorcallbacks, enhancing upload process visibility.Bug Fixes
Tests