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

Use Svelte 5 without migration #1283

Draft
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Nov 26, 2024

Fix #1174.

This PR updates our Svelte dependencies to Svelte 5 and silences the compiler warning about self-closing HTML tags (the Svelte 5 migration script will take care of them for us, but in the meantime we don't want that warning to flood the compiler output and make us miss other, more important, warnings). Some Svelte-related dependencies have also been updated to a version that officially supports Svelte 5.

The sveltekit-superforms dependency was NOT updated, because sveltekit-superforms version 2 has breaking changes to its API and we're using an API from v1 that is not present in v2. The upgrade path from v1 to v2 should be simple, but it belongs in its own PR.

@rmunn rmunn requested a review from myieye November 26, 2024 07:42
@rmunn rmunn self-assigned this Nov 26, 2024
Copy link

github-actions bot commented Nov 26, 2024

UI unit Tests

12 tests  ±0   12 ✅ ±0   0s ⏱️ ±0s
 4 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit fd453fb. ± Comparison against base commit 7eaf4f8.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 26, 2024

C# Unit Tests

98 tests  +98   98 ✅ +98   5s ⏱️ +5s
15 suites +15    0 💤 ± 0 
 1 files   + 1    0 ❌ ± 0 

Results for commit fd453fb. ± Comparison against base commit 7eaf4f8.

♻️ This comment has been updated with latest results.

We'll fix the self-closing tags when we migrate to Svelte 5; in the
meantime, we don't want these warnings to flood the compilation results
and end up hiding other, more important, warnings.
This fixes the error about TypeScript enums not being natively supported
By making the .svelte-kit directory (produced by svelte-kit sync) part
of the docker image in dev mode, we can avoid having the viewer build
fail the first time, which causes the container to restart unnecessarily.
@rmunn rmunn force-pushed the chore/use-svelte-5-without-migration branch from f2e8543 to 52a1c10 Compare November 26, 2024 08:09
@rmunn
Copy link
Contributor Author

rmunn commented Nov 26, 2024

Looks like a bunch of Typescript errors and other fixups needed: the code compiles and runs, but svelte-check isn't happy. So this will take a bit of work before it's ready to merge into develop and get a green build.

@rmunn rmunn marked this pull request as draft November 26, 2024 08:28
@rmunn rmunn removed the request for review from myieye November 26, 2024 08:28
* MaybePromise<T> = T | Promise<T> no longer exported from svelte-kit
* svelte/store no longer exports Invalidator type (now () => void)
* svelte-exmarkdown v4 needed for correct Svelte 5 component types
* svelte-exmarkdown renderer prop needs casting as Component<any> so
  that Typescript won't complain about incompatible prop types
* Components no longer have a .render method, instead you import the
  render function from Svelte and call it
* svelte/compiler no longer exports walk, we're supposed to use
  estree-walker instead
* estree-walker doesn't think nodes should have 'Element' type, as its
  types are designed for JS syntax trees, not Svelte component trees
* Could not get Typescript to accept our componentMap in emails.ts, as
  each component had incompatible props. Punted by declaring its type as
  Record<EmailTemplate, any>.
@rmunn
Copy link
Contributor Author

rmunn commented Nov 28, 2024

After much effort, I finally got all the Typescript errors fixed up in src (but not yet in viewer). Here is the commit message from da8fdd5 explaining what I fixed up:

  • MaybePromise<T> = T | Promise<T> no longer exported from svelte-kit
  • svelte/store no longer exports Invalidator type (now () => void)
  • svelte-exmarkdown v4 needed for correct Svelte 5 component types
  • svelte-exmarkdown renderer prop needs casting as Component<any> so that Typescript won't complain about incompatible prop types
  • Components no longer have a .render method, instead you import the render function from Svelte and call it
  • svelte/compiler no longer exports walk, we're supposed to use estree-walker instead
  • estree-walker doesn't think nodes should have 'Element' type, as its types are designed for JS syntax trees, not Svelte component trees
  • Could not get Typescript to accept our componentMap in emails.ts, as each component had incompatible props. Punted by declaring its type as Record<EmailTemplate, any>.

@rmunn rmunn marked this pull request as ready for review November 28, 2024 09:41
@rmunn
Copy link
Contributor Author

rmunn commented Nov 28, 2024

Linting currently failing with the following error:

Error: Error while loading rule '@typescript-eslint/await-thenable': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.

I'm not going to figure that one out today. @myieye - any ideas on how to tackle this one? Otherwise I'll see what I can do about it tomorrow.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

a couple regressions I'd like fixed, otherwise it looks good

frontend/src/routes/email/+server.ts Outdated Show resolved Hide resolved
frontend/src/routes/email/emails.ts Outdated Show resolved Hide resolved
frontend/src/lib/forms/RadioButtonGroup.svelte Outdated Show resolved Hide resolved
@hahn-kev
Copy link
Collaborator

hahn-kev commented Dec 2, 2024

pushed a single change which tells taskfile to treat the pnpm workspace file as an input for cache invalidation.

I tried it out and it works for me, there's some warnings in the logs about changing object and array params in non rune mode, but that's expected.

rmunn added 8 commits December 3, 2024 10:30
This allows us to do the renderer plugin typecast just once rather than
everywhere the Markdown component is used.
The remaining error is something we can't fix until we migrate our code
to runes mode, so we'll turn it into a warning in a separate commit.
This isn't something we can fix until we migrate our code to runes mode,
so we don't want to silence the warning but we don't want to turn it
into an error either.
Some parts of the build were erroring out due to the .svelte-kit
directory not being present. This should create that directory before
the parts of the build that need it.
@rmunn rmunn requested a review from hahn-kev December 3, 2024 06:04
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

looks good, just one minor change and it's good to go

@@ -26,7 +26,7 @@
</Button>
<Dialog {open} on:close={toggleOff} class="w-[700px]">
<div class="m-6 prose">
<Markdown md={$text} plugins={[{ renderer: { a: NewTabLinkRenderer } }]} />
<Markdown md={$text} plugins={[{ renderer: { a: NewTabLinkRenderer as unknown as Component } }]} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should use the new markdown component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new Markdown component is in the lexbox frontend and I haven't implemented it in the viewer. I'll implement a copy of it in the viewer, because we might indeed end up wanting to use it more often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit fd453fb.

That way instead of having to do a Typescript cast every time we want to
render Markdown that opens links in a new tab, we can do the cast once
and just call a different component. We've done this already for the
lexbox frontend code in src/, so we'll do the same thing for the viewer.
@hahn-kev hahn-kev assigned myieye and unassigned rmunn Dec 9, 2024
@rmunn
Copy link
Contributor Author

rmunn commented Dec 14, 2024

@myieye - I was having some trouble with Typescript types around the email template stuff, and had to change the code that pulls template out of the request (see discussion in https://github.com/sillsdev/languageforge-lexbox/pull/1283/files/37bf1d7b817e131f5c53d5c1a27313dbbc236766#diff-fff663f5949874190982a6b67bd47c95f1d0ff3466f395d2676dff90be171068 for details) because TemplateNode wasn't exposed as a type anymore. Well, with https://www.github.com/sveltejs/svelte/pull/14601 now merged into Svelte 5 as of v5.12.0, that change I made might be able to be reverted.

@hahn-kev
Copy link
Collaborator

Per our team meeting we're going to deffer this until after the workshop in February

@hahn-kev hahn-kev marked this pull request as draft December 16, 2024 08:26
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.

Svelte 5 upgrade (NPM only, no migration)
3 participants