feat: add Convex adapter #929
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/uploadthing/src/convex.ts (1)
100-100: Consider using a more descriptive variable nameUsing a single-letter variable name like
fmay reduce code readability. Consider renaming it to something more descriptive, such asuploadthingoruploadBuilder.Apply this diff to rename the variable:
-const f = createUploadthing(); +const uploadthing = createUploadthing();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
- packages/uploadthing/package.json (4 hunks)
- packages/uploadthing/src/convex.ts (1 hunks)
- packages/uploadthing/turbo.json (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/uploadthing/src/convex.ts
[failure] 107-107:
Async arrow function has no 'await' expression
🔇 Additional comments (5)
packages/uploadthing/turbo.json (1)
8-8: Addition of Convex adapter output path looks good.The addition of
"convex/**"to theoutputsarray in thebuildtask aligns well with the PR objective of introducing a new Convex adapter. This change ensures that the build process includes files from theconvexdirectory, which is essential for the new adapter's functionality.A few points to consider:
- The placement of the new output path is correct, maintaining alphabetical order within the array.
- This change should not negatively impact existing build processes but will include the new Convex-related files in the build outputs.
- Ensure that the
convexdirectory and its contents are properly structured within thepackages/uploadthingdirectory to match this configuration.To verify the existence and structure of the
convexdirectory, you can run the following script:packages/uploadthing/package.json (3)
102-111: LGTM: New export for Convex adapter added correctly.The new export for
./convexhas been added with the correct structure, including both import and require configurations. This addition aligns with the PR objective of introducing a new Convex adapter.
139-139: LGTM: Convex directory added to files array.The
convexdirectory has been correctly added to thefilesarray. This ensures that the new Convex adapter will be included in the package distribution, making it available to users.
196-196: LGTM: Convex added as an optional peer dependency.The
convexpackage has been correctly added as a peer dependency with a wildcard version (*). It has also been marked as optional in thepeerDependenciesMetasection. These changes appropriately support the new Convex adapter while keeping it optional for users who don't require it.Also applies to: 213-215
packages/uploadthing/src/convex.ts (1)
44-94: Route setup is correctly implementedThe
addUploadthingRoutesfunction effectively integrates the uploadthing routes into the Convex HTTP router. The handlers forGET,POST, andOPTIONSmethods are appropriately defined, ensuring proper handling of file uploads and CORS preflight requests.
packages/uploadthing/src/convex.ts
Outdated
| const identity = await event.auth.getUserIdentity(); | ||
| return { userId: identity?.subject ?? "nothing" }; | ||
| }) | ||
| .onUploadComplete(async (args) => { |
There was a problem hiding this comment.
Remove unnecessary async keyword from arrow function
The arrow function passed to .onUploadComplete does not contain any await expressions, so the async keyword can be removed to simplify the code.
Apply this diff to remove the unnecessary async keyword:
- .onUploadComplete(async (args) => {
+ .onUploadComplete((args) => {📝 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.
| .onUploadComplete(async (args) => { | |
| .onUploadComplete((args) => { |
🧰 Tools
🪛 GitHub Check: lint
[failure] 107-107:
Async arrow function has no 'await' expression
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
examples/backend-adapters/server/src/convex.ts (3)
1-13: LGTM with a minor typo correction.The file header and imports look good. They provide clear context and import the necessary modules. However, there's a small typo in the comment on line 7:
-// In a real app, you can wire up `cereateUploadthing` to your Convex +// In a real app, you can wire up `createUploadthing` to your Convex
7-13: Consider using a schema in production environments.The current implementation of
createUploadthing()without a schema is suitable for this example. However, for production environments, it's recommended to use a schema for increased type safety, as demonstrated in the commented-out example.In a production setting, consider uncommenting and adapting the schema implementation:
import schema from "./schema"; const f = createUploadthing({ schema });This will provide better type checking and potentially catch errors earlier in the development process.
15-24: LGTM. Consider adjusting the maxFileSize for images.The router configuration is well-structured and includes appropriate middleware for user identification. The
onUploadCompletecallback effectively returns the user ID, which is useful for tracking uploads.However, the current maxFileSize of 4MB for images might be restrictive for high-resolution photos or certain image types.
Consider adjusting the maxFileSize based on your specific use case:
imageUploader: f({ image: { maxFileSize: "10MB" } })This allows for larger image files while still maintaining a reasonable limit. Adjust the value as needed for your application's requirements.
examples/backend-adapters/server/package.json (1)
15-15: LGTM! Consider adding a development script for Convex.The addition of the Convex dependency (version ^1.16.0) aligns well with the PR objective of introducing a new Convex adapter. This change is appropriate and necessary for implementing the new adapter.
A few suggestions to consider:
- Ensure that the version pinning strategy (^1.16.0) is consistent with other dependencies in the project.
- Verify if Convex requires any peer dependencies that need to be added to the
package.json.- Consider adding a development script for Convex, similar to the other adapters. For example:
(Adjust the command as needed based on Convex's specific requirements)"dev:convex": "NODE_ENV=development tsx watch src/convex.ts"Would you like me to propose a diff for adding the Convex development script?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
- examples/backend-adapters/server/package.json (1 hunks)
- examples/backend-adapters/server/src/convex.ts (1 hunks)
- packages/uploadthing/src/convex.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
examples/backend-adapters/server/src/convex.ts (1)
26-28: LGTM. HTTP router setup and export are correct.The HTTP router is properly initialized, and the UploadThing routes are correctly added. The export statement ensures that the configured router is available for use in the Convex application.
packages/uploadthing/src/convex.ts (5)
1-31: LGTM: Imports and type definitions are well-structuredThe imports and type definitions in this segment are appropriate and provide good type safety for the Convex adapter implementation.
33-42: LGTM:createUploadthingfunction is well-implementedThe
createUploadthingfunction is correctly implemented as a generic builder for handling uploads. It provides flexibility through generics and utilizes the internalcreateBuilderfunction appropriately.
44-55: LGTM:addUploadthingRoutesfunction declaration and handler creation are correctThe
addUploadthingRoutesfunction is well-defined with appropriate type annotations. The handler creation usingmakeAdapterHandleris implemented correctly, passing the necessary arguments for the Convex adapter.
57-67: LGTM: GET and POST route definitions are correctly implementedThe GET and POST routes for '/api/uploadthing' are properly defined using the created handler. The implementation is consistent and appropriate for the upload functionality.
1-94: Overall, the Convex adapter implementation is well-structured and meets the PR objectivesThe
convex.tsfile successfully introduces a new adapter for Convex, complementing existing adapters like Express and Fastify. The implementation is straightforward and allows developers to integrate the adapter into their Convex applications seamlessly.Key points:
- The
createUploadthingfunction provides a flexible way to set up the adapter with optional schema definitions.- The
addUploadthingRoutesfunction correctly sets up the necessary HTTP routes for file uploads.- CORS handling is implemented for preflight requests.
The code addresses the PR objectives and provides a solid foundation for integrating uploadthing with Convex applications. The only suggestion for improvement is to consider extracting the CORS logic into a separate function for better readability and maintainability.
There was a problem hiding this comment.
This all looks fine, the adapter itself is super simple.
I would probably opt for a separate example that shows the actual way this is used, even if it does require a Convex account or self hosting Convex. (Our Clerk examples also require an account)
Where should I add Convex to the docs? Would another entry next to https://docs.uploadthing.com/backend-adapters/express be the right spot?
Yeah, I think that would be a reasonable place for the docs, as another section in backend-adapters
| // import schema from "./schema"; | ||
| // const f = createUploadthing({ schema }) | ||
| // ``` | ||
| const f = createUploadthing(); |
There was a problem hiding this comment.
It may be better to have a separate example showing the "real" way as you suggested in the PR comment. without it, this example doesnt really hold much value IMO
There was a problem hiding this comment.
sounds good, I'll remove this and port it to a full example.
Co-authored-by: Mark R. Florkowski <mark@ping.gg>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/uploadthing/package.json (1)
187-187: Consider adding version constraints for the Convex peer dependency.While using
"*"is consistent with other adapter dependencies, consider adding version constraints to prevent potential compatibility issues with future breaking changes in Convex.- "convex": "*", + "convex": "^1.0.0",
📜 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 (3)
examples/backend-adapters/server/package.json(1 hunks)packages/uploadthing/package.json(4 hunks)packages/uploadthing/turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/backend-adapters/server/package.json
- packages/uploadthing/turbo.json
🔇 Additional comments (3)
packages/uploadthing/package.json (3)
103-112: LGTM! Export configuration follows package conventions.The Convex adapter export configuration is well-structured and consistent with other adapters in the package.
130-130: LGTM! Files array updated correctly.The "convex" directory is properly included in the files array, maintaining alphabetical order and consistency with other adapters.
202-204: LGTM! Peer dependencies meta configuration is correct.The Convex adapter is properly marked as an optional peer dependency, consistent with other adapters in the package.
This PR adds a Convex adapter as a sibling to Express, Fastify, etc. It was pretty straightforward to add a new adapter!
Here's what it looks like for wiring it in to a Convex app. First, call
createUploadthingas always:Note that the developer can optionally pass in their Convex
schemahere for more type-safety within their handlers.Then, define the router, using the
convexCtxfunction to access the request's currentctxvalue. This is necessary for calling into other Convex functions, accessing auth and the database, etc. Under the hood, this uses a hack where we stash thectxwhen starting a request.Finally, install the routes into Convex's HTTP router with
addUploadthingRoutes:Questions
ctxvia a "global" withconvexCtxis a bit ugly. Is there a better way to pass values to the middleware and completion callbacks?examples/? It'll require setting up a Convex account (or self hosting Convex) to actually run the demo.Summary by CodeRabbit
New Features
/api/uploadthing.Enhancements
convexand its optional peer dependency.convexdirectory.convexin the backend adapter project.