Graphite icon indicating copy to clipboard operation
Graphite copied to clipboard

(frontend): Incremental migration to svelte 5

Open sm17p opened this issue 6 months ago • 29 comments

Migration Plan step by step

  • [x] Turn on dynamicCompileOptions for vite-plugin-svelte, and add files in the runes whitelist (runesGlobs)
  • [x] Run the migration script for whitelisted files
  • [x] Fix svelte/legacy depracation warnings (Avoiding createBubbler over here)
  • [x] Once all the components are runes compatible, start chiping away the remainingsvelte/legacy compat bindings
  • [ ] Add commits to .git-blame-ignore-revs for a cleaner git blame

Following the svelte 5 migration guide


Known Issues

  • [x] patchWidgetLayout doesn't update deeply nested properties reactively - 3e572c9
  • [x] ~~Graph.svelte (Node Graph) is invisible on load, displays after clicking on screen - 055b6f3~~ autofixed when rebased onto master
  • [x] NodeCatalog.svelte reactive bindings messed up for sub-menus and node creation - 461aa77
  • [x] FieldInput does not reset local component state when hit with Escape - d24c0b6

sm17p avatar Jun 25 '25 20:06 sm17p

@Keavon I've enabled runes mode for the whole project with the last commit, but still testing it. Let me know if you find anything that's breaking

sm17p avatar Jun 26 '25 07:06 sm17p

Thanks. Please keep testing it extensively and do any polishing you find 'til I get the chance to code review this. There are a few PRs in front of yours in the priority queue so it might be a couple of days, but I'll try my best to keep it from taking too long.

Keavon avatar Jun 26 '25 08:06 Keavon

I'm done with my bug fixes ✌️. Let me know if anything else is breaking as I'm not well aware of the UI hot paths and user workflow

sm17p avatar Jun 28 '25 20:06 sm17p

Could you please sync this with master?

Keavon avatar Jul 03 '25 23:07 Keavon

Synced!

sm17p avatar Jul 04 '25 06:07 sm17p

Once again please, sorry for the trouble!

Keavon avatar Jul 06 '25 04:07 Keavon

Once again please, sorry for the trouble!

No worries, have synced it again

sm17p avatar Jul 06 '25 11:07 sm17p

Could you please update it again? I think I'll be able to get to it today since it is finally roughly next in my queue.

Keavon avatar Jul 10 '25 03:07 Keavon

Synced! I noticed backend behaviour has changed?

Undo doesn't work like before. I tried it onfill property and it behaves differently on editor.graphite.rs and dev.graphite.rs. Suspecting a regression in backend code

sm17p avatar Jul 10 '25 10:07 sm17p

Thanks! What is the fill property you are referring to? The solid color given to the Fill node? From my brief comparison with master and editor, it seems they work the same. So I'm probably misinterpreting what you are discussing. Is that something where you could give some detailed reproduction steps in a newly filed issue, please? I assume that was just an observation and not tied to this Svelte upgrade specifically, right?

Keavon avatar Jul 11 '25 00:07 Keavon

This is the editore.graphite.rs vs dev.graphite.rs comparision for undo.

  1. Editor - Circle color undo's on first attempt
  2. Dev - Circle color doesn't not change even after repeated attempts, (guess is all the color updates from picker are sent to the backend which creates a long undo queue)

Correct me if I'm wrong, but dev is deployed from master branch? If yes, then this might not be related to the migration code

https://github.com/user-attachments/assets/0b8d5412-9d6a-4b30-931b-d1ff2ab790fa

sm17p avatar Jul 11 '25 01:07 sm17p

Yes, dev deploys master.

Keavon avatar Jul 11 '25 01:07 Keavon

!build

Keavon avatar Jul 11 '25 02:07 Keavon

📦 Build Complete for 46ce377df71a6ff96e77a6a2974792dbe5782192
https://00702c79.graphite.pages.dev

github-actions[bot] avatar Jul 11 '25 02:07 github-actions[bot]

What was the reason for needing to rename messages.ts to messages.svelte.ts?

Keavon avatar Jul 12 '25 01:07 Keavon

Please run cd frontend && npm run lint-fix and commit the fixes, then go through each remaining issue that hasn't been auto fixed until there are no remaining lint errors. Thank you.

Keavon avatar Jul 12 '25 01:07 Keavon

What was the reason for needing to rename messages.ts to messages.svelte.ts?

  • I made that change while I was debugging reactivity issued for the patchWidgetLayout. The problem was in the transformer of the Widget class props key where it was over-writing deeply nested objects.
  • This is however not needed anymore and I can revert back to the original. On theory it's a safe revert, it shouldn't break anything. I'll test if reactivity breaks and report back over here

Please run cd frontend && npm run lint-fix and commit the fixes, then go through each remaining issue that hasn't been auto fixed until there are no remaining lint errors. Thank you.

  • I think I broke the linter with the package update, let me check if the older version has validation for runes. If not, then I'll have to migrate the eslint config to the newer FlatConfig style, as mentionend in the svelte linter plugin migration guide https://sveltejs.github.io/eslint-plugin-svelte/migration/#2-es-lint-flat-config-only

sm17p avatar Jul 12 '25 11:07 sm17p

Runes linting test

The following code snippet should've triggered svelte/infinite-reactive-loop linter error on line 15 but it didn't.

Screenshot 2025-07-12 at 5 33 13 PM

Going by above, the lint rules for svelte 5 might not work extensively in all cases, and hence caution must be exercised to not be all reliant on linter for svelte 5 logic


Conclusion

  1. Migrating to flat config won't improve code safety so I'm just updating v2 version of eslint-plugin-svelte to ^2.46.0, and fixing the lint errors

Installed: @eslint/js@9.31.0 eslint@9.31.0 eslint-plugin-svelte@3.10.1 svelte@5.35.6 svelte-eslint-parser@1.2.0 typescript@5.8.3 typescript-eslint@8.36.0

Playground link

sm17p avatar Jul 12 '25 13:07 sm17p

Bug:

  1. TextAreaInput does not reset local state on Escape

sm17p avatar Jul 12 '25 13:07 sm17p

What was the reason for needing to rename messages.ts to messages.svelte.ts?

  • I made that change while I was debugging reactivity issued for the patchWidgetLayout. The problem was in the transformer of the Widget class props key where it was over-writing deeply nested objects.
  • This is however not needed anymore and I can revert back to the original. On theory it's a safe revert, it shouldn't break anything. I'll test if reactivity breaks and report back over here

This breaks it :( My theory was wrong, I need the widget state to be reactive to make it work with deeply nested bind's located at root level components and layers above. We need

export class Widget {
	constructor(props: WidgetPropsSet, widgetId: bigint) {
		this.props = $state(props);
		this.widgetId = $state(widgetId);
	}

	@Type(() => WidgetProps, { discriminator: { property: "kind", subTypes: [...widgetSubTypes] }, keepDiscriminatorProperty: true })
	@Transform(({ value }) => {
		if (value.kind === "PopoverButton") {
			value.popoverLayout = value.popoverLayout.map(createLayoutGroup);
		}
		return value;
	})
	props!: WidgetPropsSet;

	widgetId!: bigint;
}

to create reactive state and the if clause prevents those reactive properties to be converted back to POJO during recursion

sm17p avatar Jul 12 '25 15:07 sm17p

Bug:

  1. TextAreaInput does not reset local state on Escape

Fixed this

sm17p avatar Jul 12 '25 17:07 sm17p

Ok, nice work so far. Please keep me updated on the latest status.

Keavon avatar Jul 13 '25 21:07 Keavon

I'm gonna do a final round of testing by tonight after syncing with master. If I don't find any issues, then I'll reopen from draft again for review

sm17p avatar Jul 14 '25 04:07 sm17p

I'm done with my testing, with d24c0b6FieldInput has two different behaviours

  1. When attached with an oncommitText callback, it works in local edit mode allowing users to discard editing state on Esc
  2. Without it, it's directly bound and edits are reflective of the current state value

Aside from last bug, no new bugs have been found and therefore I'm re-opening for a review ✌️

sm17p avatar Jul 14 '25 21:07 sm17p

@Keavon can we build and push this for testing?

sm17p avatar Jul 21 '25 15:07 sm17p

!build

Keavon avatar Jul 21 '25 21:07 Keavon

📦 Build Complete for d24c0b6a44893d6654ea8f77fd98e96909bc7709
https://ee9b3112.graphite.pages.dev

github-actions[bot] avatar Jul 21 '25 21:07 github-actions[bot]

There's one task remaining which is adding the .git-blame-ignore-revs. About that, it'd be better to add in a separate PR on a need basis as the project sees fit.

Aside from that, I feel confident with the build.

I'd prefer a merge this Friday or Saturday, as I'd be available to provide with any fixes if needed anytime after that

sm17p avatar Jul 22 '25 18:07 sm17p

Would you be interested in submitting a separate PR which upgrades Graphite to Svelte 5 without also migrating to runes? This was far too large to merge in one fell swoop and it needs to be reviewed in incremental steps. The first would be to get it upgrading to Svelte 5 and successfully compiling without changing any code except that which is necessary to get it up and running.

After that, we could explore an incremental process of migrating several files at a time to runes until all are completed.

Keavon avatar Sep 06 '25 06:09 Keavon