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

Upgrade to Svelte 4 #3543

Merged
merged 51 commits into from
Dec 12, 2023
Merged

Upgrade to Svelte 4 #3543

merged 51 commits into from
Dec 12, 2023

Conversation

ericpgreen2
Copy link
Contributor

@ericpgreen2 ericpgreen2 commented Nov 22, 2023

Closes #2684

This PR upgrades Svelte 3 to Svelte 4. A handful of changes were required:

  • I ran the Svelte 4 migration script on each of our web workspaces
  • I upgraded our web dependencies to versions that support Svelte 4
  • I fixed a few cases where Svelte 4 is more strict than Svelte 3
    • ARIA roles are required for interactive elements
    • Default slot bindings are no longer exposed to named slots (see the floating-element changes)
    • The derived() store function no longer accepts falsy values (see the timeseries-data-store refactor)
  • I fixed failing unit tests
    • Some were failing due to our protobuf-base64 encoding/decoding functions. Fixed by migrating to @bufbuild/protobuf's base64 functions.
    • Some were failing due to components' onMount not getting called by vitest. Fixed by following this StackOverflow post.

@ericpgreen2 ericpgreen2 self-assigned this Nov 29, 2023
@ericpgreen2 ericpgreen2 marked this pull request as ready for review December 11, 2023 18:47
@ericpgreen2 ericpgreen2 added the dependencies Pull requests that update a dependency file label Dec 11, 2023
Copy link
Collaborator

@AdityaHegde AdityaHegde 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 mostly, just a few concerns.

@@ -68,7 +68,7 @@

<div>
{#if showForm}
<div class="mt-6 mb-4 flex flex-col gap-y-4" transition:slide>
<div class="mt-6 mb-4 flex flex-col gap-y-4" transition:slide|global>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have seen issues with global transitions. Has it been fixed in svelte4? Applies to all places where this change has been made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Svelte 4, transitions are local by default. In Svelte 3, they were global by default. So this code ensures our application behaves the same as it did with Svelte 3.

All the changes to transitions were made automatically when I ran the Svelte 4 migration script: npx svelte-migrate@latest svelte-4.

I agree we should be wary of global transitions, but let's consider application behavior changes in a separate PR.

@@ -77,7 +77,7 @@ are details left to the consumer of the component; this component should remain
{active}
/>
</Chip>
<div slot="tooltip-content" transition:fly|local={{ duration: 100, y: 4 }}>
<div slot="tooltip-content" transition:fly={{ duration: 100, y: 4 }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

|local was added to fix issues with elements lingering. Has it been fixed in svelte 4? Applies to all instances of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above – transitions are now local by default

@@ -93,6 +93,7 @@ are details left to the consumer of the component; this component should remain
<RemovableListMenu
{excludeStore}
slot="floating-element"
let:toggleFloatingElement
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the migration guide's section on default slot bindings.

Previously, we were using default slot props (like toggleFloatingElement) in named slots (like slot="floating-element"). But that's no longer allowed. Now, to migrate, I've added a named slot prop toggleFloatingElement.

@@ -41,6 +41,7 @@ and the menu closes.
<slot {handleClose} toggleMenu={toggleFloatingElement} {active} />
<Menu
slot="floating-element"
let:handleClose
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? Wont it mask the variable defined above on WithTogglableFloatingElement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above – named slots no longer have access to the default slot props

@ericpgreen2 ericpgreen2 merged commit 2d4e6cc into main Dec 12, 2023
2 checks passed
@ericpgreen2 ericpgreen2 deleted the upgrade-to-svelte-4 branch December 12, 2023 08:00
mindspank pushed a commit that referenced this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Svelte 4
3 participants