kit icon indicating copy to clipboard operation
kit copied to clipboard

feat: add Svelte 5 migration

Open dummdidumm opened this issue 1 year ago • 5 comments
trafficstars

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 test and lint the project with pnpm lint and pnpm 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 changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

dummdidumm avatar Jul 29 '24 19:07 dummdidumm

šŸ¦‹ 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

changeset-bot[bot] avatar Jul 29 '24 19:07 changeset-bot[bot]

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 😁

teemingc avatar Jul 30 '24 12:07 teemingc

Yeah or you can do node ../../../path/to/your/local/sveltekit-repo/packages/migrate/bin.js svelte-5

dummdidumm avatar Jul 30 '24 12:07 dummdidumm

Some feedback from one file with issues:

  1. 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?

  1. 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 } }]
	};
});

teemingc avatar Jul 30 '24 17:07 teemingc

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';"
}

benmccann avatar Jul 30 '24 22:07 benmccann

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/

benmccann avatar Sep 03 '24 20:09 benmccann

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),'
}

benmccann avatar Sep 03 '24 21:09 benmccann

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?

dummdidumm avatar Sep 06 '24 12:09 dummdidumm

I filed issues at https://github.com/sveltejs/svelte/issues/13178 and https://github.com/sveltejs/svelte/issues/13179

benmccann avatar Sep 09 '24 22:09 benmccann

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).

dummdidumm avatar Sep 10 '24 11:09 dummdidumm

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

benmccann avatar Sep 10 '24 14:09 benmccann

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

dummdidumm avatar Sep 10 '24 14:09 dummdidumm

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?

benmccann avatar Sep 10 '24 14:09 benmccann

  • The $$Props error 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

dummdidumm avatar Sep 10 '24 14:09 dummdidumm

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)

benmccann avatar Sep 11 '24 15:09 benmccann

Found an error case: https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAAyWJSwrDIBCGrxL-Zil1LxLoOWoXIY4wYKLEMVDEuxeb3fdoCBypwLwbjnUnGLxyhoJ885ByURSCQkn13EZpDw7TPJeYpDw9hbVG6e5wYj1fywAndtxJ_83quzfNoUNhT54Dk4eRs1L_9B8FLnc7gwAAAA==

ottomated avatar Sep 11 '24 22:09 ottomated

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

dummdidumm avatar Sep 18 '24 09:09 dummdidumm

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

Theo-Steiner avatar Sep 20 '24 07:09 Theo-Steiner

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'

Theo-Steiner avatar Sep 20 '24 07:09 Theo-Steiner

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?

Rich-Harris avatar Sep 20 '24 11:09 Rich-Harris

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

teemingc avatar Sep 21 '24 01:09 teemingc

I cleaned up the error output

before Screenshot from 2024-09-20 20-11-14

after Screenshot from 2024-09-20 20-10-36

benmccann avatar Sep 21 '24 03:09 benmccann