feat: onFileUploadComplete #1175
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes add a new optional callback, Changes
Sequence Diagram(s)sequenceDiagram
participant U as UploadButton
participant S as startUpload
participant H as useUploadThingInternal
participant F as uploadFile
participant C as onFileUploadComplete Callback
U->>S: Initiates upload with callback
S->>H: Calls uploadFiles with onFileUploadComplete
H->>F: Triggers file upload, passing callback
F-->>H: Returns file upload result
H->>C: Invokes onFileUploadComplete with file data
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
Scope: all 3 workspace projects This error happened while installing a direct dependency of /tmp/eslint/packages/uploadthing Packages found in the workspace: ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/uploadthing/src/client.ts (1)
151-169: 🛠️ Refactor suggestionUpdate the controllable upload to support the
onFileUploadCompletecallbackThe
controllableUploadfunction should also trigger theonFileUploadCompletecallback when a file upload completes. This ensures consistent behavior between the two upload methods.Update the upload promise resolution to call the callback:
void Micro.runPromiseExit(uploadEffect(file, p), { signal: deferred.ac.signal, }) .then((result) => { if (result._tag === "Success") { + opts.onFileUploadComplete?.(result.value, i, presigneds.length); return deferred.resolve(result.value); } else if (result.cause._tag === "Interrupt") { throw new UploadPausedError(); } throw Micro.causeSquash(result.cause); }) .catch((err) => { if (err instanceof UploadPausedError) return; deferred.reject(err); });
🧹 Nitpick comments (2)
packages/react/src/types.ts (1)
56-63: New callback for individual file upload completion trackingThe addition of
onFileUploadCompletecallback is a nice enhancement that allows tracking individual file uploads. This will be useful for showing progress like "Uploading file X out of Y."However, the
uploadResponseparameter is typed asany, which loses type safety. Consider using a more specific type likeUploadPutResult<TServerOutput>to maintain type safety throughout the application.onFileUploadComplete?: ( - uploadResponse: any, + uploadResponse: UploadPutResult<TServerOutput>, index: number, total: number, ) => void;packages/uploadthing/src/client.ts (1)
256-262: Document the new callback in JSDoc commentsThe new
onFileUploadCompletecallback lacks documentation explaining its purpose and parameters. Adding JSDoc comments would help users understand what this callback does and when it's called.Add JSDoc comments for the new parameter:
> & { + /** + * Callback that is called when a single file upload completes. + * This can be used to implement a "fake" upload progress indicator + * (e.g., "Uploading file 1 out of 5"). + * + * @param uploadResponse The response from the completed file upload + * @param index The index of the file that was uploaded (0-based) + * @param total The total number of files being uploaded + */ onFileUploadComplete?: ( uploadResponse: any, index: number, total: number, ) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/react/src/components/button.tsx(2 hunks)packages/react/src/types.ts(1 hunks)packages/react/src/use-uploadthing.ts(1 hunks)packages/uploadthing/src/_internal/upload-browser.ts(3 hunks)packages/uploadthing/src/client.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/uploadthing/src/_internal/upload-browser.ts (1)
packages/uploadthing/src/_internal/types.ts (1)
UploadPutResult(188-202)
🔇 Additional comments (5)
packages/react/src/use-uploadthing.ts (1)
90-94: LGTM - Properly passing the callback through the upload chainThe change correctly passes the new
onFileUploadCompletecallback from the hook props to theuploadFilesfunction, maintaining the callback chain.packages/react/src/components/button.tsx (2)
6-10: LGTM - Clean up of import statementThe replacement of commented-out imports with proper typed imports improves code clarity and maintainability.
119-121: LGTM - Proper implementation of the new callbackThe implementation correctly passes the callback parameters to the user-provided function. This maintains the contract defined in the type definition.
packages/uploadthing/src/_internal/upload-browser.ts (2)
110-113: Type definition aligns with implementationThe type definition for
onFileUploadCompletecorrectly specifies that it receives aUploadPutResult<TServerOutput>as its parameter, which is what the implementation provides.
139-139: LGTM - Efficient implementation using Micro's tap operatorThe use of
Micro.tapis an elegant way to handle the callback without affecting the main data flow. This is a clean functional programming approach.
packages/uploadthing/src/client.ts
Outdated
| > & { | ||
| onFileUploadComplete?: ( | ||
| uploadResponse: any, | ||
| index: number, | ||
| total: number, | ||
| ) => void; | ||
| }, |
There was a problem hiding this comment.
Implementation incomplete: callback needs to be passed to uploadFilesInternal
The onFileUploadComplete callback has been added to the type definition but is not being passed to the uploadFilesInternal function where it should be utilized. Without this, the callback won't be called when a file upload completes.
Add the callback to the options passed to uploadFilesInternal:
return uploadFilesInternal<TRouter, TEndpoint>(endpoint, {
...opts,
skipPolling: {} as never, // Remove in a future version, it's type right not is an ErrorMessage<T> to help migrations.
url: resolveMaybeUrlArg(initOpts?.url),
package: initOpts?.package ?? "uploadthing/client",
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
input: (opts as any).input as inferEndpointInput<TRouter[TEndpoint]>,
+ onFileUploadComplete: opts.onFileUploadComplete,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > & { | |
| onFileUploadComplete?: ( | |
| uploadResponse: any, | |
| index: number, | |
| total: number, | |
| ) => void; | |
| }, | |
| return uploadFilesInternal<TRouter, TEndpoint>(endpoint, { | |
| ...opts, | |
| skipPolling: {} as never, // Remove in a future version, it's type right not is an ErrorMessage<T> to help migrations. | |
| url: resolveMaybeUrlArg(initOpts?.url), | |
| package: initOpts?.package ?? "uploadthing/client", | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |
| input: (opts as any).input as inferEndpointInput<TRouter[TEndpoint]>, | |
| onFileUploadComplete: opts.onFileUploadComplete, | |
| }) |
🛠️ Refactor suggestion
Add the same callback to the controllableUpload function for consistency
The onFileUploadComplete callback has been added to the typedUploadFiles function but not to the controllableUpload function. According to the PR summary, this callback should be available for both functions to ensure consistent behavior across different upload methods.
Additionally, the uploadResponse parameter is typed as any, which could lead to type-safety issues. Consider using a more specific type that matches the actual upload response structure.
const controllableUpload = async <
TEndpoint extends keyof TRouter,
TServerOutput = inferEndpointOutput<TRouter[TEndpoint]>,
>(
slug: EndpointArg<TRouter, TEndpoint>,
opts: Omit<
CreateUploadOptions<TRouter[TEndpoint]>,
keyof GenerateUploaderOptions
+ > & {
+ onFileUploadComplete?: (
+ uploadResponse: ClientUploadedFileData<TServerOutput>,
+ index: number,
+ total: number,
+ ) => void;
},
) => {Also, in the typedUploadFiles function, consider using a more specific type instead of any:
onFileUploadComplete?: (
- uploadResponse: any,
+ uploadResponse: ClientUploadedFileData<inferEndpointOutput<TRouter[TEndpoint]>>,
index: number,
total: number,
) => void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > & { | |
| onFileUploadComplete?: ( | |
| uploadResponse: any, | |
| index: number, | |
| total: number, | |
| ) => void; | |
| }, | |
| // Updated controllableUpload function with onFileUploadComplete added to opts | |
| const controllableUpload = async < | |
| TEndpoint extends keyof TRouter, | |
| TServerOutput = inferEndpointOutput<TRouter[TEndpoint]>, | |
| >( | |
| slug: EndpointArg<TRouter, TEndpoint>, | |
| opts: Omit< | |
| CreateUploadOptions<TRouter[TEndpoint]>, | |
| keyof GenerateUploaderOptions | |
| > & { | |
| onFileUploadComplete?: ( | |
| uploadResponse: ClientUploadedFileData<TServerOutput>, | |
| index: number, | |
| total: number, | |
| ) => void; | |
| }, | |
| ) => { | |
| // function body | |
| }; | |
| // Updated onFileUploadComplete callback signature in typedUploadFiles function | |
| // (Assuming the following type is part of the options for typedUploadFiles) | |
| type TypedUploadFilesOptions = { | |
| // ... other options | |
| onFileUploadComplete?: ( | |
| uploadResponse: ClientUploadedFileData<inferEndpointOutput<TRouter[TEndpoint]>>, | |
| index: number, | |
| total: number, | |
| ) => void; | |
| // ... other options | |
| }; |
packages/react/src/types.ts
Outdated
| onFileUploadComplete?: ( | ||
| uploadResponse: any, | ||
| index: number, | ||
| total: number, | ||
| ) => void; |
There was a problem hiding this comment.
Not a fan of this API. I'm currently blocked on making changes to the progress callback since we chose to use positional arguments there instead of an object 😓
| onFileUploadComplete?: ( | |
| uploadResponse: any, | |
| index: number, | |
| total: number, | |
| ) => void; | |
| onFileUploadComplete?: (_: { | |
| uploadResponse: any, // <-- also, this shouldn't be any | |
| index: number, | |
| total: number, | |
| }) => void; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/uploadthing/src/types.ts (2)
12-12: Consider using a relative import path forClientUploadedFileDataThe file is importing
ClientUploadedFileDatafrom "./types" but also re-exporting it from "./_internal/shared-schemas" on line 25. This could potentially lead to circular dependencies. Consider importing it directly from "./_internal/shared-schemas" for clarity.- import type { ClientUploadedFileData } from "./types"; + import type { ClientUploadedFileData } from "./_internal/shared-schemas";
131-146: Consider enhancing the callback with index and total parametersThe
onFileUploadCompletecallback is well-documented, but it doesn't includeindexandtotalproperties that would make it easier to implement a "Uploading file X out of Y" progress indicator, which was one of the primary needs mentioned in the PR description.onFileUploadComplete?: | ((_: { /** * The response from the upload */ fileData: ClientUploadedFileData<inferEndpointOutput<TFileRoute>>; /** * The file object that was uploaded */ file: File; /** * All the files that are in this upload */ files: File[]; + /** + * The index of the current file in the upload queue (0-based) + */ + index: number; + /** + * The total number of files in the upload queue + */ + total: number; }) => void) | undefined;packages/react/src/types.ts (1)
56-59: Clarify interaction with awaitServerData optionThe documentation for
onClientUploadComplete(line 118) mentions that it's called after the serversideonUploadCompletecallback has finished ifRouteOptions.awaitServerDataistrue. It would be helpful to clarify in the documentation foronFileUploadCompletehow it interacts with theawaitServerDataoption./** * Called when a file upload is completed + * @remarks If `RouteOptions.awaitServerData` is `true`, this will be + * called after the serverside processing for this file has finished */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react/src/components/button.tsx(2 hunks)packages/react/src/types.ts(1 hunks)packages/uploadthing/src/_internal/upload-browser.ts(3 hunks)packages/uploadthing/src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/src/components/button.tsx
- packages/uploadthing/src/_internal/upload-browser.ts
🧰 Additional context used
🧬 Code Definitions (2)
packages/react/src/types.ts (1)
packages/uploadthing/src/types.ts (1)
ClientUploadedFileData(25-25)
packages/uploadthing/src/types.ts (1)
packages/uploadthing/src/_internal/shared-schemas.ts (1)
ClientUploadedFileData(96-101)
🔇 Additional comments (1)
packages/react/src/types.ts (1)
59-74: Consider enhancing the callback with index and total parametersThe
onFileUploadCompletecallback should includeindexandtotalproperties to make it easier to implement a "Uploading file X out of Y" progress indicator, which was specifically mentioned in the PR description. This matches a previous comment from juliusmarminge who had similar feedback.onFileUploadComplete?: | ((_: { /** * The response from the upload */ fileData: ClientUploadedFileData<TServerOutput>; /** * The file object that was uploaded */ file: File; /** * All the files that are in this upload */ files: File[]; + /** + * The index of the current file in the upload queue (0-based) + */ + index: number; + /** + * The total number of files in the upload queue + */ + total: number; }) => void) | undefined;
|
@juliusmarminge You're the goat. Like I said, this was to fit my particular use case, but I'm glad you've updated it to fit the general public. What is the process of getting this merged? It's something I'm waiting on to continue developing our RN app. |
|
@Roilan this pr has sparked a bunch of conversation internally around what these APIs should look like 😅 Will continue to follow up next week |
|
I'll release a canary if that helps unblock you. We will support this use case but thinking about the exact API on a broader plane |
|
@markflorkowski @juliusmarminge Haha, sorry. I'm surprised nobody has needed this before. The changes that Julius has made should unblock me. I'm okay with whatever the API for the canary release is. I can make adjustments in the future with what the official API ends up being. However, I just want to confirm that no other changes are needed "useImageUploader" in Expo since that's what I need it for. |
|
merging into unforked branch to make canary |
markflorkowski
merged commit 551afc4
into
pingdotgg:roilan/onfileuploadcomplete
I was speaking with Mark about if the upload progress is available on React Native. He mentioned implementation issues with XHR referencing expo/expo#28269.
I asked if there was a callback or hook when a file is uploaded vs all files uploaded. Having this would allow us to "fake" upload progress by saying "Uploading file 1 out of 5", etc.
This PR starts the initial callback functionality for React. It's working on my local pnpm link setup as expected. The types are iffy since I'm unsure how all the types work in the uploadthing repo, so I'd love some help with that. I'm open to changing anything that fits the general public vs just my usecase.
Summary by CodeRabbit