chore(svelte): migrate to Svelte 5 runes, upgrade deps, add browser tests#722
chore(svelte): migrate to Svelte 5 runes, upgrade deps, add browser tests#722
Conversation
🦋 Changeset detectedLatest commit: 841cb3d 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 |
size-limit report 📦
|
There was a problem hiding this comment.
Pull request overview
Modernizes the @lottiefiles/dotlottie-svelte package by moving the Svelte wrapper/demo to Svelte 5 runes, upgrading the SvelteKit/Vite toolchain, and introducing a Playwright-backed browser test suite for the wrapper’s runtime behavior.
Changes:
- Migrates
DotLottieSvelteand the demo page to Svelte 5 runes ($props,$state,$effect) and updates Svelte peer dependency to^5.0.0. - Adds Vitest Browser + Playwright (chromium) test setup with extensive component behavior coverage and coverage thresholds.
- Upgrades SvelteKit/Vite ecosystem dependencies (and lockfile) to newer major versions.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Updates lockfile to reflect SvelteKit/Vite/tooling upgrades and new test dependencies. |
packages/svelte/package.json |
Narrows Svelte peer dependency to v5 and adds browser test scripts/deps. |
packages/svelte/vitest.config.ts |
Adds Vitest browser (Playwright) configuration and coverage thresholds for the Svelte package. |
packages/svelte/setup-file.ts |
Adds Vitest browser setup and configures the WASM URL for tests. |
packages/svelte/src/lib/Dotlottie.svelte |
Migrates component implementation to Svelte 5 runes and updates reactive behavior. |
packages/svelte/src/routes/+page.svelte |
Migrates demo page to runes and DOM event attribute syntax. |
packages/svelte/tests/dotlottie-svelte.spec.ts |
Introduces comprehensive browser-based tests for prop reactivity and interactions. |
packages/svelte/tests/__snapshots__/dotlottie-svelte.spec.ts.snap |
Adds snapshot for the basic render test. |
packages/svelte/src/index.test.ts |
Removes the placeholder test. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "peerDependencies": { | ||
| "svelte": "^4.0.0 || ^5.0.0" | ||
| "svelte": "^5.0.0" | ||
| }, |
There was a problem hiding this comment.
This PR changes the published API surface (peer dependency narrowed to Svelte v5-only, plus behavior changes) and is marked as breaking in the PR description. The repo’s contributing guidelines require committing a Changeset for user-facing changes; please add a .changeset/*.md entry for @lottiefiles/dotlottie-svelte so releases/versioning are handled correctly.
| canvas?.addEventListener('mouseenter', handlePlayOnHover); | ||
| canvas?.addEventListener('mouseleave', handlePlayOnHover); | ||
|
|
||
| return () => { | ||
| canvas?.removeEventListener('mouseenter', handlePlayOnHover); | ||
| canvas?.removeEventListener('mouseleave', handlePlayOnHover); | ||
| }; |
There was a problem hiding this comment.
In this $effect, event listeners are added/removed using the reactive canvas variable, but the cleanup also references canvas directly. If canvas ever changes between registration and cleanup (e.g., conditional rendering/HMR), the cleanup may try to remove listeners from the new element and leak listeners on the old one. Capture the element in a local const currentCanvas = canvas; and use that for both add/remove inside this effect.
| if (dotLottie && typeof src === 'string' && src !== prevSrc) { | ||
| prevSrc = src; | ||
| dotLottie.load({ | ||
| src, | ||
| ...untrack(() => ({ | ||
| autoplay, | ||
| loop, | ||
| speed, | ||
| data, | ||
| renderConfig, | ||
| segment, | ||
| useFrameInterpolation, | ||
| backgroundColor, | ||
| mode, | ||
| marker, | ||
| layout, | ||
| animationId, | ||
| themeId, | ||
| stateMachineId, | ||
| stateMachineConfig | ||
| })) | ||
| }); |
There was a problem hiding this comment.
The src reactivity is now gated by typeof src === 'string'. This means changing src from a string to undefined (or otherwise clearing it) will no longer trigger a dotLottie.load(...), potentially leaving the previous animation loaded even though the prop changed. Consider handling src transitions to undefined as well (e.g., compare against prevSrc without the string-only guard, or explicitly reload when src becomes undefined).
| if (dotLottie && typeof src === 'string' && src !== prevSrc) { | |
| prevSrc = src; | |
| dotLottie.load({ | |
| src, | |
| ...untrack(() => ({ | |
| autoplay, | |
| loop, | |
| speed, | |
| data, | |
| renderConfig, | |
| segment, | |
| useFrameInterpolation, | |
| backgroundColor, | |
| mode, | |
| marker, | |
| layout, | |
| animationId, | |
| themeId, | |
| stateMachineId, | |
| stateMachineConfig | |
| })) | |
| }); | |
| if (dotLottie && src !== prevSrc) { | |
| prevSrc = src; | |
| if (typeof src === 'string') { | |
| dotLottie.load({ | |
| src, | |
| ...untrack(() => ({ | |
| autoplay, | |
| loop, | |
| speed, | |
| data, | |
| renderConfig, | |
| segment, | |
| useFrameInterpolation, | |
| backgroundColor, | |
| mode, | |
| marker, | |
| layout, | |
| animationId, | |
| themeId, | |
| stateMachineId, | |
| stateMachineConfig | |
| })) | |
| }); | |
| } else { | |
| // Handle transitions where src is cleared or becomes non-string | |
| dotLottie.load({ | |
| ...untrack(() => ({ | |
| autoplay, | |
| loop, | |
| speed, | |
| data, | |
| renderConfig, | |
| segment, | |
| useFrameInterpolation, | |
| backgroundColor, | |
| mode, | |
| marker, | |
| layout, | |
| animationId, | |
| themeId, | |
| stateMachineId, | |
| stateMachineConfig | |
| })) | |
| }); | |
| } |
| $effect(() => { | ||
| if ( | ||
| dotLottie && | ||
| (typeof data === 'string' || typeof data === 'object') && | ||
| data !== undefined && | ||
| data !== prevData | ||
| ) { | ||
| prevData = data; | ||
| dotLottie.load({ | ||
| data, | ||
| ...untrack(() => ({ | ||
| src, | ||
| autoplay, | ||
| loop, | ||
| speed, | ||
| renderConfig, | ||
| segment, | ||
| useFrameInterpolation, | ||
| backgroundColor, | ||
| mode, | ||
| marker, | ||
| layout, | ||
| animationId, | ||
| themeId, | ||
| stateMachineId, | ||
| stateMachineConfig | ||
| })) | ||
| }); | ||
| } |
There was a problem hiding this comment.
This data change effect skips reloads when data becomes undefined (because of data !== undefined), so clearing the data prop may not revert to loading from src and can leave stale content. Also, typeof data === 'object' includes null, but Config['data'] doesn’t allow null (it’s string | ArrayBuffer | Record<string, unknown>). Consider reacting to data !== prevData even when data is undefined, and explicitly excluding null.
| playOnHover: true, | ||
| dotLottieRefCallback, | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test reads dotLottieRefCallback.mock.calls[0] immediately after render(...) without waiting for the callback to be invoked. Since the component creates the DotLottie instance in a $effect after the canvas is bound, the callback can be async and this can be flaky. Add a waitFor that asserts the callback was called before accessing mock.calls[0].
| await vi.waitFor(() => { | |
| expect(dotLottieRefCallback).toHaveBeenCalled(); | |
| }); |
|
@harshmandan, we're planning to drop Svelte 4 support. I'm wondering if this change would break your apps ? |
|
Hey @theashraf, yeah all the apps are fully migrated to svelte 5. |
…ests Rewrite DotLottieSvelte component to use Svelte 5 runes API ($props, $effect, $state) replacing legacy Svelte 4 patterns (export let, $:, on:event). Narrow svelte peer dependency from ^4 || ^5 to ^5.0.0. Add comprehensive browser test suite (16 tests) using vitest-browser-svelte and @testing-library/user-event with Playwright, replacing the minimal placeholder test. Upgrade SvelteKit toolchain: vite ^5 -> ^7.3.1, @sveltejs/vite-plugin-svelte ^4 -> ^6.2.4, @sveltejs/adapter-auto ^3 -> ^7.0.1, svelte-check ^3.6 -> ^4.4.4, publint ^0.1.9 -> ^0.3.17, and bump @sveltejs/kit and svelte within range.
theashraf
force-pushed
the
chore/svelte-upgrade-deps-and-runes
branch
from
997394a to
841cb3d
Compare
Description
Modernize the
@lottiefiles/dotlottie-sveltepackage with three related changes:Svelte 5 runes migration — Rewrite
DotLottieSveltecomponent from legacy Svelte 4 patterns (export let,$:reactive statements,on:eventdirectives) to Svelte 5 runes API ($props(),$effect(),$state,onclick). The+page.sveltedemo is also migrated. Narrows svelte peer dependency from^4.0.0 || ^5.0.0to^5.0.0.Comprehensive browser test suite — Replace the minimal placeholder test with 16 tests using
vitest-browser-svelte,@testing-library/user-event, and Playwright (chromium). Tests cover rendering, prop reactivity (loop, speed, mode, useFrameInterpolation, backgroundColor, marker, layout, renderConfig, animationId, src, data), playOnHover behavior, and state machine integration. Coverage thresholds: 95% statements/functions/lines, 89% branches.Dependency upgrades — Major bumps for the SvelteKit toolchain:
vite^5.0.13→^7.3.1@sveltejs/vite-plugin-svelte^4.0.0→^6.2.4@sveltejs/adapter-auto^3.0.0→^7.0.1svelte-check^3.6.0→^4.4.4publint^0.1.9→^0.3.17@sveltejs/kitandsvelteupdated within rangePackage(s) affected
@lottiefiles/dotlottie-web(core)@lottiefiles/dotlottie-react@lottiefiles/dotlottie-vue@lottiefiles/dotlottie-svelte@lottiefiles/dotlottie-solid@lottiefiles/dotlottie-wcType of change
Checklist