chore: revert url changes #1128
Conversation
🦋 Changeset detectedLatest commit: 096f1d8 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
|
|
Warning Rate limit exceeded@juliusmarminge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces modifications to the UploadThing library's URL handling mechanism, primarily by adding a new Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
🪧 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 (
|
📦 Bundle size comparison
|
More templates
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/uploadthing/test/__test-helpers.ts (1)
47-49: Consider consolidating URL pattern functions.The
appUrlPatternandfileUrlPatternfunctions share similar logic. Consider consolidating them into a single function with a type parameter to reduce code duplication.-export const appUrlPattern = (appId = testToken.decoded.appId) => - new RegExp(`^${UTFS_URL}/a/${appId}/.+$`); -export const fileUrlPattern = () => new RegExp(`^${UTFS_URL}/f/.+$`); +export const createUrlPattern = (type: 'app' | 'file', appId = testToken.decoded.appId) => + new RegExp(`^${UTFS_URL}/${type === 'app' ? `a/${appId}` : 'f'}/.+$`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/uploadthing/src/_internal/types.ts(1 hunks)packages/uploadthing/src/_internal/upload-browser.ts(1 hunks)packages/uploadthing/src/sdk/utils.ts(1 hunks)packages/uploadthing/test/__test-helpers.ts(4 hunks)packages/uploadthing/test/client.browser.test.ts(4 hunks)packages/uploadthing/test/sdk.live.test.ts(7 hunks)packages/uploadthing/test/sdk.test.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: analyze-bundle (current-pr)
- GitHub Check: typecheck
- GitHub Check: e2e-node (backend-adapters)
- GitHub Check: lint
- GitHub Check: e2e-node (minimal-pagedir)
- GitHub Check: e2e-node (minimal-appdir)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (10)
packages/uploadthing/test/__test-helpers.ts (1)
34-37: Verify PR objective alignment.The addition of
UTFS_URLand subsequent URL changes appear to introduce new URL patterns rather than reverting them, which seems to contradict the PR title "chore: revert url changes".Could you clarify if this is intentionally introducing new URL patterns or if this should be reverting previous URL changes?
packages/uploadthing/src/sdk/utils.ts (1)
150-150: LGTM!The addition of
ufsUrlto the return object is consistent with the existing pattern.packages/uploadthing/src/_internal/upload-browser.ts (2)
143-143: LGTM!The addition of
ufsUrlis consistent with the changes in other files.
Line range hint
1-1: Review PR title and description alignment.The PR title suggests this is reverting URL changes, but the code changes appear to be:
- Adding a new
ufsUrlproperty- Deprecating existing URL properties
- Introducing new URL patterns
This seems more like a feature enhancement than a revert. Consider:
- Updating the PR title to reflect the actual changes
- Adding a clear migration guide for the URL property deprecations
- Documenting the rationale for these changes in the PR description
packages/uploadthing/test/sdk.live.test.ts (2)
7-12: LGTM! Import changes look good.The addition of
appUrlPatternandufsUrlPatternimports aligns with the URL pattern changes being tested.
72-74: Consistent implementation of URL pattern assertions.The URL pattern assertions have been consistently updated across all test cases to verify three distinct URLs:
url: UsingfileUrlPattern()appUrl: UsingappUrlPattern(appId)ufsUrl: UsingufsUrlPattern(appId)This change ensures comprehensive validation of the URL structure in the upload response.
Also applies to: 108-110, 139-141, 164-166, 207-209, 249-251
packages/uploadthing/test/client.browser.test.ts (2)
20-20: LGTM! Import changes are consistent.The addition of URL pattern imports maintains consistency with other test files.
Also applies to: 28-28
146-147: URL pattern assertions properly implemented in browser tests.The browser-specific upload tests have been correctly updated to verify all three URL patterns in the upload response.
Also applies to: 191-192, 227-228
packages/uploadthing/test/sdk.test.ts (2)
16-16: LGTM! Import changes maintain consistency.The URL pattern imports are consistent with the changes in other test files.
Also applies to: 18-18, 23-23
55-57: Comprehensive URL pattern validation across SDK tests.The URL pattern assertions have been systematically updated across all test cases, including:
- Single file uploads
- Batch uploads
- Error scenarios
- File uploads from URLs
This ensures consistent validation of the URL structure in all upload responses.
Also applies to: 171-173, 305-307, 343-345, 367-369
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/uploadthing/src/_internal/shared-schemas.ts(1 hunks)packages/uploadthing/src/_internal/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/_internal/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: typecheck
- GitHub Check: lint
- GitHub Check: e2e-node (backend-adapters)
- GitHub Check: e2e-node (minimal-pagedir)
- GitHub Check: analyze-bundle (current-pr)
- GitHub Check: e2e-node (minimal-appdir)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
packages/uploadthing/src/_internal/shared-schemas.ts (1)
84-84: Verify consumers ofUploadedFileDataschema.Let's ensure that all consumers of this schema are prepared for the new
ufsUrlproperty.✅ Verification successful
Documentation-only change with proper deprecation notice
This is a safe change that only adds a deprecation notice for a future version (v9) while maintaining backward compatibility. The notice correctly guides users to use the
ufsUrlfield as an alternative.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of UploadedFileData to verify compatibility # Find direct references to UploadedFileData echo "=== Direct references to UploadedFileData ===" rg "UploadedFileData" --type ts # Find potential type usage through ClientUploadedFileData echo -e "\n=== References to ClientUploadedFileData ===" rg "ClientUploadedFileData" --type ts # Find potential implementations or extensions echo -e "\n=== Potential implementations or extensions ===" ast-grep --pattern 'extends $_ implements UploadedFileData'Length of output: 8163
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/uploadthing/test/request-handler.test.ts (1)
414-416: Consider adding URL format validation tests.While the URL construction is consistent, consider adding test cases that specifically validate:
- The format of the generated URLs
- The deprecation warnings for
urlandappUrl- The accessibility of the generated URLs
Example test case:
it("should warn when accessing deprecated url properties", () => { const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const data = new UploadedFileData({ url: `${UTFS_URL}/f/test.png`, appUrl: `${UTFS_URL}/a/${testToken.decoded.appId}/f/test.png`, ufsUrl: `https://${testToken.decoded.appId}.${UFS_HOST}/f/test.png`, // ... other required properties }); // Access deprecated properties expect(data.url).toBeDefined(); expect(data.appUrl).toBeDefined(); // Verify deprecation warnings expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining("'url' is deprecated") ); expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining("'appUrl' is deprecated") ); consoleSpy.mockRestore(); });packages/uploadthing/src/_internal/upload-server.ts (1)
45-60: LGTM! Consider enhancing the deprecation notice with version information.The implementation of deprecation warnings using getters is a clean approach. However, to help users better plan their migration, consider including the current version in the deprecation message.
- "`file.url` is deprecated and will be removed in uploadthing v9. Use `file.ufsUrl` instead.", + "`file.url` is deprecated in uploadthing v${version} and will be removed in v9. Use `file.ufsUrl` instead.",packages/uploadthing/src/_internal/handler.ts (1)
339-353: Consider extracting deprecation messages to constants.The implementation is correct, but the deprecation messages are duplicated across files. Consider extracting these messages to a shared constants file to ensure consistency and easier maintenance.
Create a new constants file:
// src/_internal/constants.ts export const DEPRECATION_MESSAGES = { FILE_URL: "`file.url` is deprecated and will be removed in uploadthing v9. Use `file.ufsUrl` instead.", FILE_APP_URL: "`file.appUrl` is deprecated and will be removed in uploadthing v9. Use `file.ufsUrl` instead.", } as const;Then update the usage:
- "`file.url` is deprecated and will be removed in uploadthing v9. Use `file.ufsUrl` instead.", + DEPRECATION_MESSAGES.FILE_URL,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/uploadthing/src/_internal/handler.ts(2 hunks)packages/uploadthing/src/_internal/logger.ts(1 hunks)packages/uploadthing/src/_internal/upload-browser.ts(2 hunks)packages/uploadthing/src/_internal/upload-server.ts(2 hunks)packages/uploadthing/test/request-handler.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/_internal/upload-browser.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: analyze-bundle (current-pr)
- GitHub Check: typecheck
- GitHub Check: e2e-node (backend-adapters)
- GitHub Check: e2e-node (minimal-pagedir)
- GitHub Check: lint
- GitHub Check: e2e-node (minimal-appdir)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
packages/uploadthing/test/request-handler.test.ts (1)
414-416: LGTM! URL construction pattern is consistently implemented.The URL construction pattern is consistently applied across all test cases, properly handling the transition to the new URL structure with the addition of
ufsUrl.Also applies to: 452-454, 466-468, 500-502
packages/uploadthing/src/_internal/logger.ts (1)
90-93: LGTM! Clean implementation of the deprecation logger.The implementation provides clear visual indicators and consistent formatting for deprecation warnings.
| file: requestInput.file, | ||
| file: { | ||
| ...requestInput.file, | ||
| get url() { |
There was a problem hiding this comment.
bit more encouragement to get people migrating faster
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/uploadthing/src/_internal/deprecations.ts (1)
1-4: Add JSDoc documentation for better maintainability.The implementation looks good and follows best practices. However, adding JSDoc documentation would improve maintainability and provide better IDE support.
Consider adding documentation:
+/** + * Logs a deprecation warning to the console. + * @param message - The deprecation message to display + * @example + * logDeprecationWarning('Property "url" is deprecated. Use "ufsUrl" instead.'); + */ export const logDeprecationWarning = (message: string) => { // eslint-disable-next-line no-console console.warn(`⚠️ [uploadthing] ${message}`); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/uploadthing/src/_internal/deprecations.ts(1 hunks)packages/uploadthing/src/_internal/handler.ts(2 hunks)packages/uploadthing/src/_internal/upload-browser.ts(2 hunks)packages/uploadthing/src/_internal/upload-server.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/uploadthing/src/_internal/upload-server.ts
- packages/uploadthing/src/_internal/handler.ts
- packages/uploadthing/src/_internal/upload-browser.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: e2e-node (backend-adapters)
- GitHub Check: e2e-node (minimal-pagedir)
- GitHub Check: e2e-node (minimal-appdir)
- GitHub Check: analyze-bundle (current-pr)
- GitHub Check: typecheck
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
Summary by CodeRabbit
New Features
ufsUrlproperty for uploaded files.urlandappUrlproperties.Deprecation Notice
urlandappUrlwill be removed in uploadthing v9.ufsUrlfor file URLs going forward.Improvements