Skip to content

(frontend): Incremental migration to svelte 5 #2755

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

sm17p
Copy link

@sm17p sm17p commented Jun 25, 2025

Migration Plan step by step

  • Turn on dynamicCompileOptions for vite-plugin-svelte, and add files in the runes whitelist (runesGlobs)
  • Run the migration script for whitelisted files
  • Fix svelte/legacy depracation warnings (Avoiding createBubbler over here)
  • 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

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

@sm17p
Copy link
Author

sm17p commented Jun 26, 2025

@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

@Keavon
Copy link
Member

Keavon commented Jun 26, 2025

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.

@sm17p sm17p force-pushed the migrate-svelte-5 branch 2 times, most recently from aa22517 to e80d066 Compare June 26, 2025 14:26
@sm17p sm17p force-pushed the migrate-svelte-5 branch from 7917f6f to fec6147 Compare June 28, 2025 12:24
@sm17p
Copy link
Author

sm17p commented Jun 28, 2025

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 sm17p force-pushed the migrate-svelte-5 branch from f278827 to 13580bb Compare June 29, 2025 21:16
@Keavon
Copy link
Member

Keavon commented Jul 3, 2025

Could you please sync this with master?

@sm17p sm17p force-pushed the migrate-svelte-5 branch from a2ec24d to 7f6b529 Compare July 4, 2025 06:35
@sm17p
Copy link
Author

sm17p commented Jul 4, 2025

Synced!

@Keavon
Copy link
Member

Keavon commented Jul 6, 2025

Once again please, sorry for the trouble!

@sm17p sm17p force-pushed the migrate-svelte-5 branch 2 times, most recently from c0225d2 to d0c34e0 Compare July 6, 2025 11:40
@sm17p
Copy link
Author

sm17p commented Jul 6, 2025

Once again please, sorry for the trouble!

No worries, have synced it again

@Keavon
Copy link
Member

Keavon commented Jul 10, 2025

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 Keavon force-pushed the master branch 4 times, most recently from ec51271 to e025103 Compare July 10, 2025 05:48
@sm17p sm17p force-pushed the migrate-svelte-5 branch from d0c34e0 to 75b6f50 Compare July 10, 2025 10:48
@sm17p
Copy link
Author

sm17p commented Jul 10, 2025

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

@Keavon
Copy link
Member

Keavon commented Jul 11, 2025

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?

@sm17p
Copy link
Author

sm17p commented Jul 11, 2025

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

editor_VS_dev_UNDO_behaviour.mp4

@Keavon
Copy link
Member

Keavon commented Jul 11, 2025

Yes, dev deploys master.

@Keavon
Copy link
Member

Keavon commented Jul 11, 2025

!build

Copy link

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

@Keavon
Copy link
Member

Keavon commented Jul 12, 2025

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

@sm17p
Copy link
Author

sm17p commented Jul 14, 2025

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 sm17p force-pushed the migrate-svelte-5 branch 4 times, most recently from 3ee3daa to cef7569 Compare July 14, 2025 21:17
@sm17p
Copy link
Author

sm17p commented Jul 14, 2025

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 sm17p marked this pull request as ready for review July 14, 2025 21:20
@sm17p sm17p force-pushed the migrate-svelte-5 branch 2 times, most recently from 84c60d2 to d24c0b6 Compare July 19, 2025 17:23
@sm17p
Copy link
Author

sm17p commented Jul 21, 2025

@Keavon can we build and push this for testing?

@Keavon
Copy link
Member

Keavon commented Jul 21, 2025

!build

Copy link

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

@sm17p sm17p force-pushed the migrate-svelte-5 branch from e200fea to 6ac1736 Compare July 22, 2025 18:04
@sm17p
Copy link
Author

sm17p commented Jul 22, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants