Skip to content
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: add Svelte 5 migration #12519

Merged
merged 20 commits into from
Sep 27, 2024
Merged

feat: add Svelte 5 migration #12519

merged 20 commits into from
Sep 27, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jul 29, 2024

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
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • 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

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

Copy link

changeset-bot bot commented Jul 29, 2024

🦋 Changeset detected

Latest commit: b9e7c6f

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

@eltigerchino
Copy link
Member

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 😁

@dummdidumm
Copy link
Member Author

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

colors
.bold()
.yellow(
'\nDetected Svelte 3. We recommend running the `svelte-4` migration first (`npx svelte-migrate svelte-4`).\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you sure they're not running svelte 1 or 2? 🤣

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what-year-is-it.jpg

@eltigerchino
Copy link
Member

eltigerchino commented Jul 30, 2024

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

@benmccann
Copy link
Member

benmccann commented Jul 30, 2024

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. immich-app/immich#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
Copy link
Member

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 😆

https://www.reddit.com/r/sveltejs/comments/1778zq4/interface_props_mentioned_in_the_svelte_5_post/

@benmccann
Copy link
Member

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

@dummdidumm
Copy link
Member Author

dummdidumm commented Sep 6, 2024

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?

Copy link
Member

@Conduitry Conduitry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has some commented-out code with no indication of why it was commented out or when you might want to uncomment it - which seems fishy. Does it just need to be deleted entirely?

@Theo-Steiner
Copy link
Contributor

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
Copy link
Contributor

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'

@Rich-Harris
Copy link
Member

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?

@eltigerchino
Copy link
Member

eltigerchino commented Sep 21, 2024

The lint test is failing for projects created with svelte 5 but I think that can be fixed in another PR.

EDIT: #12697 should fix them

@benmccann
Copy link
Member

I cleaned up the error output

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:svelte-migrate Issues related to the svelte-migrate package svelte 5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants