kit
kit copied to clipboard
feat: add Svelte 5 migration
Successfully tested on kit.svelte.dev, but another manual test from a non-windows machine on a different project would be good
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
- [ ] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
- [x] This message body should clearly illustrate what problems it solves.
- [x] Ideally, include a test that fails without this PR but passes with it.
Tests
- [x] Run the tests with
pnpm testand lint the project withpnpm lintandpnpm check
Changesets
- [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.
Edits
- [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
š¦ Changeset detected
Latest commit: b9e7c6fef9440cbce2c3a50e56bc945cdc7e8eb8
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| svelte-migrate | Minor |
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
Do I just need to link svelte-migrate globally and then run it inside a project? Iād try it on my Macbook just out of curiosity š
Yeah or you can do node ../../../path/to/your/local/sveltekit-repo/packages/migrate/bin.js svelte-5
Some feedback from one file with issues:
- Previously, you could do this for uninitialised reactive state:
let containerElement: HTMLElement;
But when $state is added by the migration, we get a type error.
// Type 'HTMLElement | undefined' is not assignable to type 'HTMLElement'.
// Type 'undefined' is not assignable to type 'HTMLElement'.
let containerElement: HTMLElement = $state();
I think this is fine, but is there anything more we should be doing?
- Also, a reactive statement conversion that went wrong, but I'm not sure how to correct it:
before
const extraOpts = {
modifiers: [{ name: 'offset', options: { offset: [0, 3] } }]
};
export let offset: [number, number] = [0, 3];
// re-assign it the value of offset whenever offset changes
$: extraOpts.modifiers[0].options.offset = offset;
after
const extraOpts = $state({
modifiers: [{ name: 'offset', options: { offset: [0, 3] } }]
});
interface Props {
text: string;
offset?: [number, number];
children?: import('svelte').Snippet;
}
let { text, offset = [0, 3], children }: Props = $props();
// error
let extraOpts.modifiers[0].options.offset = $derived(offset);
Should it just be like this?
interface Props {
text: string;
offset?: [number, number];
children?: import('svelte').Snippet;
}
let { text, offset = [0, 3], children }: Props = $props();
const extraOpts = $derived(() => {
return {
modifiers: [{ name: 'offset', options: { offset } }]
};
});
I got a handful of errors like the below when testing against the web folder of immich. I also got one about some invalid HTML which printed a similar stacktrace instead of a more helpful error with details about which file the error was in. Though I was eventually able to figure that one out and send them a fix, so it's possible it will have been fixed if they merge before you check out the project. https://github.com/immich-app/immich/pull/11465
Error while migrating Svelte code
InternalCompileError: `$$Props` is an illegal variable name. To reference a global variable called `$$Props`, use `globalThis.$$Props`
at e (node_modules/svelte/src/compiler/errors.js:29:8)
at Module.global_reference_invalid (node_modules/svelte/src/compiler/errors.js:162:2)
at analyze_component (node_modules/svelte/src/compiler/phases/2-analyze/index.js:282:6)
at migrate (node_modules/svelte/src/compiler/migrate/index.js:42:20)
at transform_svelte_code (../../kit/kit/packages/migrate/migrations/svelte-5/migrate.js:60:10)
at ../../kit/kit/packages/migrate/migrations/svelte-5/index.js:139:36
at update_js_file (../../kit/kit/packages/migrate/utils.js:325:19)
at migrate (../../kit/kit/packages/migrate/migrations/svelte-5/index.js:139:5)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
code: 'global_reference_invalid',
filename: 'migrate.svelte',
position: [ 987, 994 ],
start: { line: 45, column: 7, character: 987 },
end: { line: 45, column: 14, character: 994 },
frame: '43: \n' +
'44: <script lang="ts">\n' +
'45: type $$Props = Props;\n' +
' ^\n' +
'46: \n' +
"47: export let type: $$Props['type'] = 'button';"
}
It fails to migrate $$Props, which I didn't know existed until I tried to run it on a project that had it and it bombed out :laughing:
https://www.reddit.com/r/sveltejs/comments/1778zq4/interface_props_mentioned_in_the_svelte_5_post/
The Unexpected token error could be clearer. It says line: 60, column: 33, but it's quite buried and I missed that. I only saw it after sticking that file into the REPL and seeing the error message there. I also wonder if this particular case should be an unexpected token since it seems Svelte 4 can parse it?
Error updating src/lib/components/admin-page/jobs/jobs-panel.svelte: InternalCompileError: Unexpected token
at e (node_modules/svelte/src/compiler/errors.js:29:8)
at Module.js_parse_error (node_modules/svelte/src/compiler/errors.js:968:2)
at Parser.acorn_error (node_modules/svelte/src/compiler/phases/1-parse/index.js:148:5)
at read_script (node_modules/svelte/src/compiler/phases/1-parse/read/script.js:39:10)
at element (node_modules/svelte/src/compiler/phases/1-parse/state/element.js:281:20)
at new Parser (node_modules/svelte/src/compiler/phases/1-parse/index.js:85:12)
at parse (node_modules/svelte/src/compiler/phases/1-parse/index.js:298:17)
at migrate (node_modules/svelte/src/compiler/migrate/index.js:29:16)
at transform_svelte_code (kit/packages/migrate/migrations/svelte-5/migrate.js:59:9)
at kit/packages/migrate/migrations/svelte-5/index.js:138:36 {
code: 'js_parse_error',
filename: 'migrate.svelte',
position: [ 1822, 1822 ],
start: { line: 60, column: 33, character: 1822 },
end: { line: 60, column: 33, character: 1822 },
frame: '58: \n' +
'59: $: jobDetails = <Partial<Record<JobName, JobDetails>>>{\n' +
'60: [JobName.ThumbnailGeneration]: {\n' +
' ^\n' +
'61: icon: mdiFileJpgBox,\n' +
'62: title: $getJobName(JobName.ThumbnailGeneration),'
}
It fails to migrate $$Props
Can you give an example and post it over at the Svelte repo? This is likely something we need to fix within the migrate function from svelte/compiler
I also wonder if this particular case should be an unexpected token since it seems Svelte 4 can parse it?
Same here: can you give the full code snippet?
I filed issues at https://github.com/sveltejs/svelte/issues/13178 and https://github.com/sveltejs/svelte/issues/13179
This is ready for a final review, would like to give people the opporunity to try this out on next versions before the 5.0 release. Any migration hickups are likely to be solved within Svelte itself (the migrate function).
I think we could arguably print a better error when encountering an error for the parse case or $$Props cases above. Ideally we're able to fix those in Svelte and typescript-acorn, but it could be a belt and suspenders thing in case there are other errors we're not aware of yet. Like maybe do something nicer with the code position and avoid printing the stack trace unless there's a verbose flag or something
But overall this looks good to me. Any cleanup could potentially be done in a follow up to let folks start testing this out
I think we could arguably print a better error when encountering an error for the parse case or $$Props cases above
These are problems that need solving within Svelte itself, not here
These are problems that need solving within Svelte itself, not here
The problems themselves should be solved in Svelte, but I'm talking about more general handling of unexplained exceptions. Wouldn't that have to be done here?
- The
$$Propserror is pretty descriptive, but wrong -> needs fixing. - Any parser error by definition is not descriptive, but also can't be explained here - how would we know the reason if the Svelte parser itself doesn't know? -> can't be fixed
I wasn't suggesting to change the text of the error messages, but to nicely print the col/row number which are buried right now and to suppress the stack trace (maybe show it if --verbose), which makes it quite hard to read when there are lots of errors (e.g. immich had lots of errors because they used $$Props in several places)
Found an error case: https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAAyWJSwrDIBCGrxL-Zil1LxLoOWoXIY4wYKLEMVDEuxeb3fdoCBypwLwbjnUnGLxyhoJ885ByURSCQkn13EZpDw7TPJeYpDw9hbVG6e5wYj1fywAndtxJ_83quzfNoUNhT54Dk4eRs1L_9B8FLnc7gwAAAA==
Note to self: In case we want to eagerly insteall in the user's node modules we can use https://github.com/antfu-collective/package-manager-detector
Also shared this on discord, but running this migration tool on a codebase at work (using pnpm dlx "sveltejs/kit#svelte-5-migration&path:packages/migrate" svelte-5 for anyone curious) it tripped over nested css.
In our case we're using scss, but it seems to also trip over (now) valid css nesting:
Error updating src/components/***.svelte: InternalCompileError: Expected a valid CSS identifier
obviously can't copy the stack trace, but the code snippet it tripped over was sth like this:
.my-class {
&--with-modifier {
but also:
div {
& my-element {
did not work despite being valid css
regarding the recommended next steps displayed after the migration completes:
Recommended next steps:
1: git commit -m "migration to Svelte 5"
2: Review the migration guide at https://svelte.dev/docs/svelte/v5-migration-guide
3: Read the updated docs at https://svelte.dev/docs/svelte
Run git diff to review changes.
this probably should also include installing the updated dependencies?
1. install the updated dependencies: 'npm i' / 'pnpm i'
Do we want to add a message along the lines of
This migration is experimental ā please report any bugs to https://github.com/sveltejs/svelte/issues
until it's been more widely tested, or are we reasonably confident in it?
The lint test is failing for projects created with svelte 5 but I think that can be fixed in another PR.
EDIT: https://github.com/sveltejs/kit/pull/12697 should fix them
I cleaned up the error output
before
after