-
Notifications
You must be signed in to change notification settings - Fork 326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Migrate Svelte package to Svelte 5 #1049
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces significant updates to the documentation and implementation of the UploadThing library for SvelteKit. Key changes include modifications to code examples, emphasizing the use of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (20)
packages/svelte/src/lib/component/Cancel.svelte (1)
9-9
: LGTM! Proper usage of Svelte 5's $props rune.The implementation correctly uses the new $props rune with proper typing.
Consider adding a brief comment explaining the $props rune usage for developers who might be new to Svelte 5, as this is a breaking change from previous versions.
+ // Using Svelte 5's $props rune for reactive props with TypeScript support let { cn, className }: Props = $props();
packages/svelte/src/lib/component/Uploader.svelte (1)
11-13
: Props interface aligns with Svelte 5 best practices.The Props interface is well-structured for use with the $props rune. Consider adding JSDoc comments to document the interface, especially since this is a public-facing component.
+/** + * Props for the Uploader component + * @template TRouter - The FileRouter type + * @template TEndpoint - The endpoint key from TRouter + */ interface Props { + /** The uploader configuration for handling file uploads */ uploader: UploadthingComponentProps<TRouter, TEndpoint>; }examples/minimal-sveltekit/src/routes/+page.svelte (2)
Line range hint
22-29
: Consider consolidating upload handlers and document breaking changes.The code currently maintains two separate upload handlers (
uploader
andut
) with similar configurations. While this change aligns with Svelte 5's requirements regarding runes and destructuring, consider:
- Consolidating error handling and completion callbacks to avoid duplicate alerts
- Adding a comment explaining why destructuring is not used (breaking change)
Consider refactoring to:
- const ut = createUploadThing("videoAndImage", { - /** - * @see https://docs.uploadthing.com/api-reference/react#useuploadthing - */ - onClientUploadComplete: (res) => { - console.log(`onClientUploadComplete`, res); - alert("Upload Completed"); - }, - }); + // Note: Don't destructure the returned object to maintain reactivity in Svelte 5 + const ut = createUploadThing("videoAndImage", { + onUploadAborted: () => { + alert("Upload Aborted"); + }, + onClientUploadComplete: (res) => { + console.log(`onClientUploadComplete`, res); + alert("Upload Completed"); + }, + onUploadError: (error: Error) => { + alert(`ERROR! ${error.message}`); + }, + });
41-43
: Implement the missing file handling logic.The comment "Do something with files" suggests that additional file processing logic needs to be implemented before starting the upload.
Would you like me to help implement the file handling logic or create a GitHub issue to track this task?
packages/svelte/src/lib/utils/createFetch.svelte.ts (2)
9-19
: LGTM! Consider adding JSDoc documentation.The
FetchResponse
class implementation effectively uses Svelte 5's$state
rune and provides a clean interface for state management. The method chaining pattern withreturn this
is a nice touch.Consider adding JSDoc documentation to describe the class purpose and method behavior:
+/** + * Manages the state of a fetch operation with reactive properties using Svelte 5 runes. + * @template T The type of data being fetched + */ class FetchResponse<T> implements State<T> { data?: T = $state(); error?: Error = $state(); type: "fetched" | "error" | "loading" = $state("loading"); + /** + * Updates the state properties atomically + * @returns this - For method chaining + */ set({ data, error, type }: State<T>) {
Line range hint
24-50
: Document migration steps for consumersThe transition from stores to
FetchResponse
requires clear migration guidance for consumers.Consider adding a comment block explaining the migration:
+/** + * Creates a reactive fetch operation using Svelte 5 runes. + * @breaking-change v5.0.0 - Returns FetchResponse instance instead of a store + * + * Migration guide: + * Before: + * ```ts + * const { data, error } = createFetch(url); + * ``` + * + * After: + * ```ts + * const response = createFetch(url); + * // Access reactive properties directly + * $: ({ data, error } = response); + * ``` + */ export function createFetch<T = unknown>(url?: string, options?: RequestInit) {packages/svelte/src/lib/create-uploadthing.svelte.ts (1)
139-141
: Enhance deprecation notice with migration exampleWhile the deprecation is properly marked, consider enhancing the deprecation notice with a migration example.
/** - * @deprecated Use `routeConfig` instead + * @deprecated Use `routeConfig` instead. + * @example + * // Before + * const { permittedFileInfo } = useUploadThing(...) + * // After + * const { routeConfig } = useUploadThing(...) */packages/svelte/src/lib/component/UploadButton.svelte (3)
82-82
: Enhance the comment about rune limitationsThe current comment could be more descriptive about why destructuring isn't possible with runes.
-// Cannot destructure when using runes state +// Objects returned from functions using runes must be accessed as a whole to maintain reactivity
Line range hint
100-111
: Consider enhancing error handlingWhile the basic error handling is in place for
UploadAbortedError
, consider adding:
- Timeout handling
- Network error handling
- File size error handling
const uploadFiles = (files: File[]) => { const input = "input" in uploader ? uploader.input : undefined; ut.startUpload(files, input).catch((e) => { if (e instanceof UploadAbortedError) { void uploader.onUploadAborted?.(); + } else if (e instanceof TypeError) { + // Handle network errors + void uploader.onUploadError?.(new Error('Network error occurred')); + } else if (e.message?.includes('size')) { + // Handle file size errors + void uploader.onUploadError?.(new Error('File size exceeds limit')); } else { throw e; } }); };
116-121
: Consider a more descriptive variable nameInstead of adding a comment about naming conflicts with the $state rune, consider using a more descriptive variable name that naturally avoids the conflict.
-// Cannot be called just "state" because the compiler confuses it with the $state rune -let uploadState = $derived.by(() => { +let buttonState = $derived.by(() => { if (disabled) return "disabled"; if (!disabled && !ut.isUploading) return "ready"; return "uploading"; });docs/src/app/(docs)/getting-started/svelte/page.mdx (3)
Line range hint
231-247
: Add a code comment explaining the non-destructuring patternThe example correctly shows storing the result in
ut
variable instead of destructuring. To make it more educational, consider adding a comment explaining why:// Don't destructure to maintain reactivity with Svelte 5 runes const ut = createUploadThing("imageUploader", { onClientUploadComplete: () => { alert("Upload Completed"); }, });
252-259
: Enhance the warning with a concrete exampleThe warning about stores vs runes is helpful, but consider adding a before/after example to make it more concrete:
<Warning> Previous versions of `@uploadthing/svelte` used stores as a way to pass state around. New versions use [runes](https://svelte.dev/docs/svelte/what-are-runes) instead, so things like destructuring the object returned by `createUploadThing` is not advised any more as it breaks reactivity. + For example: + ```ts + // ❌ Don't do this (breaks reactivity): + const { startUpload } = createUploadThing(...) + + // ✅ Do this instead: + const ut = createUploadThing(...) + // Then use ut.startUpload + ``` </Warning>
268-303
: Document the state object typeThe examples show usage of various state properties (
isUploading
,fileTypes
,ready
), but the type of the state object isn't documented. Consider adding:### UploadButton content customization + ```ts + // The state object passed to snippets has this type: + interface UploadState { + isUploading: boolean; + fileTypes: string[]; + ready: boolean; + // ... other properties + } + ``` +packages/svelte/src/lib/component/create-dropzone.svelte.ts (1)
301-309
: Rename getters to avoid naming conflicts and improve clarityHaving getters with the same names as variables (
rootProps
,inputProps
,state
) can be confusing and may lead to unintended side effects. Renaming the getters can enhance readability and maintainability.Consider applying this diff:
return { - get rootProps() { + getRootProps() { return rootProps; }, - get inputProps() { + getInputProps() { return inputProps; }, - get state() { + getState() { return dropzoneState.state; }, };packages/svelte/src/lib/component/UploadDropzone.svelte (6)
77-77
: Simplify prop destructuring without$props()
The use of
$props()
for destructuring props may be unnecessary here. Since you're defining component props via theProps
type, you can destructure them directly without calling$props()
.Consider simplifying the code by directly destructuring the props:
let { uploader, onChange, onDrop, disabled, class: className, appearance, uploadIcon, label, allowedContent, button, }: Props;
83-84
: Avoid unnecessary use of$derived
for static configurationUsing
$derived
for non-reactive or static values may be unnecessary. Ifuploader.config
doesn't change over time, you can assign its properties directly.Refactor to directly assign the configuration values:
let { mode = "auto", appendOnPaste = false, cn = defaultClassListMerger } = uploader.config ?? {};
87-90
: Use standard variable declarations instead of$state()
The
$state()
rune is typically used for creating reactive state variables. If reactivity isn't required, you can declare variables directly without$state()
.Simplify the variable declarations:
let rootRef: HTMLElement | undefined; let inputRef: HTMLInputElement | undefined; let uploadProgress = 0; let files: File[] = [];
135-137
: Avoid unnecessary$derived
for static valuesIf
ut.routeConfig
is not expected to change reactively, using$derived
may be unnecessary for extractingfileTypes
andmultiple
.Assign the values directly:
let { fileTypes, multiple } = generatePermittedFileTypes(ut.routeConfig);
140-149
: Confirm the necessity of$derived
in dropzone creationUsing
$derived
fordropzone
may be unnecessary if the returned object doesn't need to be reactive.Consider initializing
dropzone
without$derived
:const dropzone = createDropzone({ rootRef, inputRef, onDrop: onDropCallback, multiple, accept: fileTypes ? generateClientDropzoneAccept(fileTypes) : undefined, disabled, });
151-151
: Simplify theready
variable declarationIf
fileTypes.length
isn't reactive, using$derived
might not be needed for theready
variable.Declare
ready
directly:let ready = fileTypes.length > 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
docs/src/app/(docs)/getting-started/svelte/page.mdx
(3 hunks)examples/minimal-sveltekit/package.json
(1 hunks)examples/minimal-sveltekit/src/routes/+page.svelte
(2 hunks)packages/svelte/package.json
(2 hunks)packages/svelte/src/lib/component/Cancel.svelte
(1 hunks)packages/svelte/src/lib/component/UploadButton.svelte
(10 hunks)packages/svelte/src/lib/component/UploadDropzone.svelte
(9 hunks)packages/svelte/src/lib/component/Uploader.svelte
(1 hunks)packages/svelte/src/lib/component/create-dropzone.svelte.ts
(11 hunks)packages/svelte/src/lib/create-uploadthing.svelte.ts
(5 hunks)packages/svelte/src/lib/index.ts
(1 hunks)packages/svelte/src/lib/utils/createFetch.svelte.ts
(2 hunks)tooling/eslint-config/package.json
(1 hunks)tooling/eslint-config/svelte.js
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tooling/eslint-config/package.json
🔇 Additional comments (26)
packages/svelte/src/lib/index.ts (2)
6-7
:
Document breaking changes in createDropzone usage
According to the PR objectives, there are important breaking changes in createDropzone
:
- Functions using runes outside Svelte components require getters for reactivity
- Returned objects can't be destructured anymore
Consider adding migration notes in the documentation to help users update their code:
+ // Old usage (no longer works)
+ const { upload, maxSize } = createDropzone();
+
+ // New usage
+ const dropzone = createDropzone();
+ // Access properties through the object
+ dropzone.upload();
+ dropzone.maxSize;
1-2
: Verify path consistency across the codebase
The addition of the .svelte
extension might indicate internal implementation changes related to Svelte 5 migration. Let's ensure this pattern is consistently applied.
tooling/eslint-config/svelte.js (1)
13-15
: LGTM! Consider adding a comment about experimental status.
The addition of experimentalGenerics
support aligns well with the PR objective of migrating to Svelte 5 and removing workarounds for generics in Svelte components.
Let's verify if this is the recommended configuration for Svelte 5:
Consider adding a comment to document the experimental nature of this feature:
parserOptions: {
parser: "@typescript-eslint/parser",
+ // Enable experimental generics support for Svelte 5 migration
svelteFeatures: {
experimentalGenerics: true,
},
},
✅ Verification successful
LGTM! Consider adding a comment about the experimental status.
The addition of experimentalGenerics
support aligns well with the PR objective of migrating to Svelte 5 and removing workarounds for generics in Svelte components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for documentation about experimentalGenerics in the repository
# and related packages
# Search for any documentation about experimentalGenerics
rg -i "experimentalGenerics|svelte.*generic"
# Check package.json files for svelte-eslint-parser version
fd package.json --exec jq -r '.dependencies["svelte-eslint-parser"] // .devDependencies["svelte-eslint-parser"] // empty'
Length of output: 240
Script:
#!/bin/bash
# Description: Verify usage of svelte-eslint-parser and documentation for experimentalGenerics
# Search for usage of svelte-eslint-parser in the codebase
rg -i "svelte-eslint-parser"
# Search for documentation mentioning experimentalGenerics within the docs directory
rg -i "experimentalGenerics" docs/
Length of output: 305
packages/svelte/src/lib/component/Cancel.svelte (1)
4-7
: LGTM! Props type definition is well-structured.
The Props type definition clearly specifies the component's property interface.
Let's verify this pattern is consistently applied across other Svelte components:
✅ Verification successful
Consistent Props type definitions confirmed.
The Props type definition pattern is consistently applied across all Svelte components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent Props type definition pattern across Svelte components
# Expected: Similar Props type definitions in other components
# Search for Props type definitions in Svelte components
ast-grep --pattern 'type Props = {
$$$
}'
# Backup search for any type definitions in Svelte components
rg -t svelte "type Props ="
Length of output: 106
Script:
#!/bin/bash
# Description: Check for consistent Props type definition pattern across Svelte components
# Search for Props type definitions in Svelte components using ast-grep
ast-grep --pattern 'type Props = {
$$$
}'
# Search for Props type definitions in .svelte files using rg with glob pattern
rg "type Props = {" --glob "*.svelte"
Length of output: 177
Script:
#!/bin/bash
# Description: Check for consistent Props type definition pattern across Svelte components
# Search for Props type definitions in Svelte components using ast-grep
ast-grep --pattern 'type Props = {
$$$
}'
# Search for Props type definitions in .svelte files using rg with glob pattern and escaped braces
rg "type Props = \{" --glob "*.svelte"
Length of output: 295
packages/svelte/src/lib/component/Uploader.svelte (2)
1-9
: LGTM! Clean type constraints and imports.
The script tag properly defines the generic constraints, and imports are well-organized.
15-15
: Verify migration guide coverage for $props usage.
The transition from export let
to $props
rune is correctly implemented. However, this is a breaking change that needs clear documentation.
examples/minimal-sveltekit/package.json (1)
16-16
: Verify compatibility with Svelte 5
The upgrade to @uploadthing/svelte@7.2.0
needs to be verified for full Svelte 5 compatibility, especially considering the breaking changes mentioned in the PR objectives regarding runes and component props.
examples/minimal-sveltekit/src/routes/+page.svelte (1)
38-46
: Verify the impact of breaking changes in file upload handling.
The change from destructured startUpload
to ut.startUpload
is part of a breaking change. Ensure that all file upload handlers in the codebase are updated accordingly.
✅ Verification successful
File upload handling updates verified successfully.
- Updated
ut.startUpload
in:packages/svelte/src/lib/component/UploadButton.svelte
packages/svelte/src/lib/component/UploadDropzone.svelte
examples/minimal-sveltekit/src/routes/+page.svelte
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining destructured usage of startUpload
rg "const.*\{.*startUpload.*\}.*=.*createUploadThing"
# Search for direct startUpload calls that might need updating
rg "startUpload\(" --type svelte
Length of output: 233
Script:
#!/bin/bash
# Search for any remaining destructured usage of startUpload in .svelte files
rg "const.*\{.*startUpload.*\}.*=.*createUploadThing" --glob "*.svelte"
# Search for direct startUpload calls that might need updating in .svelte files
rg "startUpload\(" --glob "*.svelte"
Length of output: 405
packages/svelte/src/lib/utils/createFetch.svelte.ts (1)
25-25
: Breaking Change: Verify consumer code updates
The function now returns a FetchResponse
instance instead of a readonly store. This is a breaking change that requires consumer code updates.
Let's check for existing usage patterns:
Also applies to: 50-50
packages/svelte/package.json (1)
40-48
: Verify compatibility between updated dev dependencies
The major version bump of @sveltejs/vite-plugin-svelte
to v4 and updates to other dev dependencies need verification for compatibility, especially:
- @sveltejs/vite-plugin-svelte ^4.0.0 with SvelteKit ^2.8.0
- svelte-check ^4.0.7 with Svelte ^5.1.15
#!/bin/bash
# Description: Check for known compatibility issues
# Check package.json files in examples for working combinations
echo "Checking example projects for dependency combinations..."
fd -g "package.json" -x grep -l "vite-plugin-svelte\|svelte-check\|@sveltejs/kit" {} \; -x cat {}
# Check for recent issues related to these versions
gh api graphql -f query='
{
search(query: "repo:sveltejs/kit is:issue state:open svelte 5 vite-plugin-svelte", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
url
createdAt
}
}
}
}
'
packages/svelte/src/lib/create-uploadthing.svelte.ts (3)
Line range hint 53-66
: State management simplified with Svelte 5 runes
The transition from stores to $state
rune simplifies the state management while maintaining reactivity. The initialization of upload progress tracking is well-structured.
118-121
: LGTM: Proper cleanup of upload state
The implementation ensures all state variables are properly reset after upload completion or failure.
22-31
: Implementation aligns with Svelte 5 patterns
The changes correctly implement Svelte 5's rune system with $derived
and follow the getter pattern required for rune usage outside components. This is a breaking change as noted in the PR objectives.
Let's verify no direct destructuring of the config is used in the codebase:
packages/svelte/src/lib/component/UploadButton.svelte (4)
6-6
: LGTM: Proper migration to Svelte 5 snippets
The import of Snippet
type aligns with the PR objective of migrating from named slots to snippets for content customization.
37-53
: LGTM: Well-structured type definitions
The type definitions are properly migrated to Svelte 5:
ButtonAppearance
type provides good style customization optionsProps
type correctly usesSnippet
type for customizable content sections
55-70
: LGTM: Proper implementation of Svelte 5 runes
The migration to Svelte 5 runes is well implemented:
- Uses $props() for prop management
- Uses $state for reactive variables
- Uses $derived for computed values
Also applies to: 79-80
Line range hint 1-281
: LGTM: Successful migration to Svelte 5
The component has been successfully migrated to Svelte 5 with all objectives met:
- Proper use of new runes ($props, $state, $derived)
- Migration from slots to snippets
- Maintained functionality while improving type safety
docs/src/app/(docs)/getting-started/svelte/page.mdx (1)
Line range hint 1-311
: Documentation accurately reflects Svelte 5 migration changes
The documentation successfully covers all the major changes related to the Svelte 5 migration, including the transition to runes and snippets, with clear examples and appropriate warnings about breaking changes.
packages/svelte/src/lib/component/create-dropzone.svelte.ts (2)
Line range hint 49-56
: Ensure reactive updates to props
are properly handled
When creating props
with $derived
, it's important to confirm that any changes to _props
are reflected in props
. Verify that $derived
appropriately handles reactivity in this context.
Consider using $props
or an alternative reactive approach if $derived
does not meet the required reactivity.
- const props = $derived({
+ const props = $props({
disabled: false,
maxSize: Number.POSITIVE_INFINITY,
minSize: 0,
multiple: true,
maxFiles: 0,
..._props,
});
29-31
:
Potential loss of reactivity due to the use of untrack
Using untrack
within the reducible
function may prevent the Svelte reactivity system from tracking changes to state
, which could lead to unexpected behavior if dependent reactive declarations are not updated as expected. Consider revisiting the use of untrack
to ensure that state changes are properly tracked.
To verify the impact, ensure that reactive statements depending on state
are updating correctly when dispatch
is called.
packages/svelte/src/lib/component/UploadDropzone.svelte (6)
97-112
: Ensure proper handling of the AbortController
When managing the upload cancellation with AbortController
, verify that acRef
is correctly instantiated and that the abort mechanism functions as intended.
Test the upload process to ensure that calling acRef.abort()
successfully cancels the upload and triggers the onUploadAborted
callback if provided.
191-199
: Validate the use of $derived
for styleFieldArg
Ensure that $derived
is necessary for styleFieldArg
and that it updates correctly with reactive dependencies.
Confirm that all properties within styleFieldArg
are reactive and that using $derived
provides the intended reactivity.
201-206
: Check correctness of $derived.by
usage for uploadState
Using $derived.by
with a function should correctly compute uploadState
. Verify that this usage aligns with the Svelte 5 runes API.
Ensure that uploadState
updates appropriately when any of its dependencies (disabled
, ready
, ut.isUploading
) change.
Line range hint 222-242
: Confirm correct usage of {@render}
with snippets
The use of {@render uploadIcon(styleFieldArg)}
assumes that uploadIcon
is a Snippet
. This is the correct approach for rendering dynamic content passed as snippets.
280-284
: Validate CSS class combinations and Tailwind classes
The conditional classes applied to the button element use Tailwind CSS classes. Ensure that all classes are correctly defined and that they produce the desired styling, especially the dynamic classes based on uploadState
.
Review the class combinations for each uploadState
to confirm they match design specifications:
"cursor-not-allowed bg-blue-400"
for"disabled"
and"readying"
."bg-blue-400 after:absolute after:left-0 after:h-full after:bg-blue-600 after:content-[''] ${progressWidths[uploadProgress]}"
for"uploading"
."bg-blue-600"
for"ready"
.
295-312
: Ensure button text reflects current upload state accurately
The button text changes based on the uploadState
and mode
. Verify that the conditional rendering covers all possible states and displays the appropriate text to the user.
Test each state to confirm:
- Displays
"Loading..."
whenuploadState === "readying"
. - Shows upload progress or a cancel option when
"uploading"
. - Displays the number of files ready to upload in manual mode.
- Provides the correct prompt in other states.
"@sveltejs/adapter-auto": "^3.3.1", | ||
"@sveltejs/kit": "^2.8.0", | ||
"@sveltejs/vite-plugin-svelte": "^4.0.0", | ||
"svelte": "^5.1.15", | ||
"svelte-check": "^4.0.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Known Issues with Current Svelte 5 Versions
Several open bug reports exist for Svelte 5.0, indicating potential instability. Thorough testing is recommended before proceeding.
🔗 Analysis chain
Review Svelte 5 ecosystem version compatibility
The updates to Svelte ecosystem packages represent a major version jump:
- Svelte 5.1.15 (major version upgrade)
- svelte-check 4.0.7 (major version upgrade)
- vite-plugin-svelte 4.0.0 (major version upgrade)
These versions align with the Svelte 5 migration objective, but there are some considerations:
- Svelte 5 is still in beta/preview
- The combination of these specific versions should be tested thoroughly
Consider adding a note in the project documentation about:
- The experimental nature of Svelte 5
- Potential breaking changes in future releases
- Migration steps for users updating from Svelte 4
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify compatibility between selected versions
# Check if these specific versions are recommended together
curl -s https://api.github.com/repos/sveltejs/svelte/releases | \
jq -r '.[] | select(.tag_name | contains("5.1.15")) | .body' | \
grep -i "compatibility"
# Check for known issues with this version combination
gh api graphql -f query='
{
repository(owner: "sveltejs", name: "svelte") {
issues(first: 5, states: OPEN, labels: ["5.0", "bug"]) {
nodes {
title
url
labels(first: 5) {
nodes {
name
}
}
}
}
}
}'
Length of output: 1441
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@uploadthing/svelte", | |||
"version": "7.1.1", | |||
"version": "7.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a major version bump due to breaking changes
The PR introduces breaking changes (rune usage requirements, snippet props instead of slots), but the version is only bumped from 7.1.1 to 7.2.0. According to semver, breaking changes should trigger a major version bump.
- "version": "7.2.0",
+ "version": "8.0.0",
📝 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.
"version": "7.2.0", | |
"version": "8.0.0", |
@@ -29,23 +29,23 @@ | |||
"!dist/**/*.spec.*" | |||
], | |||
"peerDependencies": { | |||
"svelte": "^4.0.0 || ^5.0.0", | |||
"svelte": "^5.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Breaking change for Svelte 5 upgrade is not documented in the changelog
Please ensure that the migration to Svelte 5 and the dropping of Svelte 4 support are clearly documented in the changelog or release notes.
🔗 Analysis chain
Verify breaking change documentation for Svelte 5 requirement
The package now requires Svelte 5 and drops support for Svelte 4. This is a breaking change that needs to be clearly documented in the changelog/release notes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if breaking change is documented in changelog
# Look for CHANGELOG.md in the repository
if [[ -f CHANGELOG.md ]]; then
echo "Checking CHANGELOG.md for Svelte 5 migration notes..."
rg -i "svelte.*5|breaking.*change" CHANGELOG.md
fi
# Check if there's any migration guide
fd -g "*MIGRATION*.md"
Length of output: 4687
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rabbit is correct - this PR would require a major bump to follow semver. are the gains here worth it? my immediate thought is this would limit adoption as I assume not a whole lot of users are using Svelte 5 yet?
I guess uploadthing users are more of the bleeding-edge type of users so they're more likely to upgrade sooner, but still a question worth asking. cc @markflorkowski
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tough because it also implies that Svelte package is ahead of other frameworks, where functionally it is not.
Is it possible to add support for svelte 5 while maintaining back compat? I assume not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the concerns, and while I do believe many svelte users have adopted v5, I have no idea how big the adoption might truly be. 🤔 maybe create a new package, something like "svelte-next"? That would keep back compat and make a smoother transition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don’t love the idea of introducing a new package for this. I’ll take a look at other packages tomorrow to see how others are handling this. Might be best to just bite the bullet and do the major… really not sure.
get isUploading() { | ||
return isUploading; | ||
}, | ||
get routeConfig() { | ||
return routeConfig.config; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change: Values must be accessed through getters
This implementation follows Svelte 5's requirements for rune usage outside components. However, this is a breaking change that affects any code destructuring these values.
Consider adding a code comment explaining why getters are necessary and how to properly access these values. Example:
+ // Note: Due to Svelte 5 runes, these values must be accessed through getters
+ // and cannot be destructured. Example:
+ // ❌ const { isUploading } = useUploadThing(...)
+ // ✅ const ut = useUploadThing(...); ut.isUploading
return {
startUpload,
get isUploading() {
📝 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.
get isUploading() { | |
return isUploading; | |
}, | |
get routeConfig() { | |
return routeConfig.config; | |
}, | |
// Note: Due to Svelte 5 runes, these values must be accessed through getters | |
// and cannot be destructured. Example: | |
// ❌ const { isUploading } = useUploadThing(...) | |
// ✅ const ut = useUploadThing(...); ut.isUploading | |
get isUploading() { | |
return isUploading; | |
}, | |
get routeConfig() { | |
return routeConfig.config; | |
}, |
<!-- svelte-ignore a11y_click_events_have_key_events --> | ||
<!-- svelte-ignore a11y_no_noninteractive_element_interactions --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Address ignored accessibility warnings properly
Instead of ignoring accessibility warnings, consider implementing proper keyboard interaction handling.
-<!-- svelte-ignore a11y_click_events_have_key_events -->
-<!-- svelte-ignore a11y_no_noninteractive_element_interactions -->
<label
+ on:keydown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ e.currentTarget.click();
+ }
+ }}
+ role="button"
+ tabindex="0"
class={cn(
Committable suggestion skipped: line range outside the PR's diff.
<Warning> | ||
Previous versions of `@uploadthing/svelte` used named slots for content | ||
customization. New versions use | ||
[snippets](https://svelte.dev/docs/svelte/snippet) instead. | ||
</Warning> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add migration guidance to the warning
The warning about slots to snippets transition could be more helpful with migration examples:
<Warning>
Previous versions of `@uploadthing/svelte` used named slots for content
customization. New versions use
[snippets](https://svelte.dev/docs/svelte/snippet) instead.
+
+ To migrate your code:
+ ```svelte
+ <!-- Before (slots) -->
+ <UploadButton {uploader}>
+ <svelte:fragment slot="button">
+ Upload
+ </svelte:fragment>
+ </UploadButton>
+
+ <!-- After (snippets) -->
+ <UploadButton {uploader}>
+ {#snippet button(state)}
+ Upload
+ {/snippet}
+ </UploadButton>
+ ```
</Warning>
ondragover: onDragOver, | ||
ondragleave: onDragLeave, | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
ondrop: onDropCb as any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid using as any
and disabling ESLint rules for event handlers
Casting event handlers to any
and disabling ESLint rules can mask type issues and reduce code safety. It's better to correctly type the event handlers to maintain type safety.
Apply this diff to properly type the event handlers:
- // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
- ondrop: onDropCb as any,
+ ondrop: onDropCb,
- // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
- onchange: onDropCb as any,
+ onchange: onDropCb,
Ensure that onDropCb
is correctly typed to match the expected event handler signatures.
Also applies to: 294-294
accept: acceptAttr!, | ||
minSize: props.minSize, | ||
maxSize: props.maxSize, | ||
multiple: props.multiple, | ||
maxFiles: props.maxFiles, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential null or undefined acceptAttr
The non-null assertion acceptAttr!
assumes that acceptAttr
is always defined. If props.accept
is not provided, acceptAttr
could be undefined
, which may cause runtime errors. It's safer to handle this possibility explicitly.
Apply this diff to provide a default value for acceptAttr
:
- accept: acceptAttr!,
+ accept: acceptAttr ?? '',
📝 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.
accept: acceptAttr!, | |
minSize: props.minSize, | |
maxSize: props.maxSize, | |
multiple: props.multiple, | |
maxFiles: props.maxFiles, | |
accept: acceptAttr ?? '', | |
minSize: props.minSize, | |
maxSize: props.maxSize, | |
multiple: props.multiple, | |
maxFiles: props.maxFiles, |
This updates the svelte package to make full use of the new v5 features.
Props
Component props use the
$props
rune. Makes them easier to type and manage compared to the bunch ofexport let
declarations.State
$state
and$derived
runes are used not only inside components but also in plain ts files, replacing stores in these cases.createDropzone
function is vastly simplified and it's now more in line with the other packagesCaveats*
$state
rune to avoid this, but I would say it's the same DX hit. This is a breaking change for people using thecreateUploadThing
function directly, I have added a note in the docs for this.Snippets
Components now accept (as props) snippets instead of named slots. Also a breaking change and I added a note in the docs for this as well.
Other changes include the syntax change for declaring event listeners and no workarounds needed any more to work with generics in svelte components.
Summary by CodeRabbit
New Features
UploadButton
andUploadDropzone
components.Bug Fixes
Chores