svelte-ux icon indicating copy to clipboard operation
svelte-ux copied to clipboard

Svelte 5 compatibility

Open techniq opened this issue 1 year ago • 21 comments

Issue to track any Svelte 5 compatibility issues. Hoping close to 0 changes are required in Svelte UX to support 5.


⚠️ PENDING: sveld (API docs) not compatible

issue: #435


⚠️ PENDING: MultiSelect hangs browser (MultiSelectField / MultiSelectMenu appear to work fine)

example: https://svelte5-docs.svelte-ux.pages.dev/docs/components/SelectField (see screenshot)


✅ FIXED: Form $: dispatch('change', ...) called immediately (not after change)

examples: svelte 4, svelte 5 fix: https://github.com/techniq/svelte-ux/commit/f419435edbf4f0c004896e87e92b9f7ba770bcc9


✅ FIXED: SelectField selection not working (bind:value issue?)

example: https://svelte5-docs.svelte-ux.pages.dev/docs/components/SelectField fix: https://github.com/techniq/svelte-ux/pull/448


✅ FIXED: Duration Cannot access 'timer' before initialization

example: https://svelte5-docs.svelte-ux.pages.dev/docs/components/Duration fix: https://github.com/techniq/svelte-ux/pull/447


✅ WORKAROUND: Dialog/Drawer can only be opened once (likely portal related). Removes subsequent elements. Issue with portal'd content and multiple elements in same

issue: https://github.com/sveltejs/svelte/issues/12440, see also: https://github.com/sveltejs/svelte/issues/12427#issuecomment-2226916099 example: https://svelte5-docs.svelte-ux.pages.dev/docs/components/Dialog workaround: Place each portal'd content into separate {#if} block (commit)


✅ WORKAROUND: Portal actions timing is different with available DOM

issue: https://github.com/techniq/svelte-ux/issues/437 fix: add explicit document.body for example, likely only affects docs (presence of .PortalTarget element)

✅ WONTFIX: <tr> is invalid inside <table>

issue: https://github.com/sveltejs/svelte/issues/12090 fix: add explilcit <tbody>


✅ FIXED: can only receive attributes, not directives

issue: https://github.com/sveltejs/svelte/issues/10382 fix: https://github.com/sveltejs/svelte/pull/10391


✅ FIXED: type attribute must be a static text value if input uses two-way binding

Fixed via: https://github.com/sveltejs/svelte/pull/9713 Reported in discord. If ran within the Svelte 5 beta (ex. 5.0.0-next.15), currently Input throws a 'type' attribute must be a static text value if input uses two-way binding error

image


✅ FIXED: let directive on <svelte:self>

issue: https://github.com/sveltejs/svelte/issues/9710 fix: https://github.com/sveltejs/svelte/pull/9727

techniq avatar Nov 29 '23 15:11 techniq

I found a fix for this, I made a PR #145. There is one more problem with TreeList.svelte, but I am hoping to get early feedback on this fix first!

CropWatchDevelopment avatar Nov 30 '23 01:11 CropWatchDevelopment

Thanks @CropWatchDevelopment, but it looks like this is a regression with the Svelte 5 compiler (see issue and Twitter/X discussion, including confirmation by Simon (Svelte/Vercel team).

I also found a workaround using <svelte:element>, although I would prefer to wait for it to be fixed upstream (which it should be). It looks like Svelte 3/4 had an explicit check for bind:this that likely was overlooked as part of the Svelte 5 rewrite, so likely the fix will be similar (and hopefully as simple).

Would it be OK if we wait a bit to see if this is resolved upstream before we consider a workaround?

I'm curious what you found in TreeList.

techniq avatar Nov 30 '23 02:11 techniq

100% Yes OK to wait until later. After checking the Svelte repo and seeing all of the v5 issues logged. I am thinking I may wait a few months even before making the move. It seems like v5 may take a bit be fully worked out :) I will pull down the PR, but Hope to help out in the future!

CropWatchDevelopment avatar Nov 30 '23 06:11 CropWatchDevelopment

100% Yes OK to wait until later. After checking the Svelte repo and seeing all of the v5 issues logged. I am thinking I may wait a few months even before making the move. It seems like v5 may take a bit be fully worked out :) I will pull down the PR, but Hope to help out in the future!

FWIW, the bind:this issue is already fixed and should be on 5.0.0-next.16 once released (already on the preview REPL). I was going to test my StackBlitz until I realize .16 wasn't published yet.

TBH, that's my plan ATM. As much as I love all the great things coming in Svelte 5, I personally will wait closer to the official release before I focus too much on it. My plan is for Svelte UX (and LayerChart) 1.0 releases to be fully compatibility with Svelte 3/4/5, and 2.0 I'll probably start embracing some of the Svelte 5 features (runes, snippets, etc) which will likely break 3/4 compatibility, but as long as 5 is a drop in replacement for 3/4, I think that's a fair tradeoff.

Just the speed/size improvements in 5 running non-rune mode is exciting.

Thanks for the PR and discussion.

techniq avatar Nov 30 '23 12:11 techniq

Filed https://github.com/sveltejs/svelte/issues/10382 because SelectField.svelte fails to compile with latest Svelte 5.

CleanShot 2024-02-02 at 20 42 27

dimfeld avatar Feb 03 '24 06:02 dimfeld

Thanks @dimfeld for submitting the upstream issue with repo. I've updated this issue's description to make it easy to track any known Svelte 5 compatibility issues. The Svelte team has been amazingly fast resolving these thus far.

techniq avatar Feb 03 '24 13:02 techniq

Haven't tried it but I think this will hit svelte-ux as well https://github.com/sveltejs/svelte/pull/10395

edit: this one is fixed now

dimfeld avatar Feb 05 '24 03:02 dimfeld

Identified the root cause of the Dialog/Drawer problem (portal'd content with elements in the same conditional) - https://github.com/sveltejs/svelte/issues/12440

techniq avatar Jul 14 '24 22:07 techniq

Workaround identified - https://github.com/techniq/svelte-ux/commit/54d9d39ee7bdb712f6c2e85faea1701aae9fe3ba

techniq avatar Jul 15 '24 21:07 techniq

The SelectField bug is quite strange... The option setting code gets called a bunch of times, not sure if that's normal or not. But at some point updateSelected is called from the reactive statement, and options is an empty array instead of the actual options. That's why things get cleared.

dimfeld avatar Jul 31 '24 04:07 dimfeld

Oh actually I figured it out... The problem is that a bunch of the SelectField controls are binding to the same value, and one of them is using options={optionsAsync}, which starts out empty. So at some point when that SelectField runs its reactivity it clears the value since its options don't contain the number, which then propagates back to the first SelectField.

Commenting out the example with optionsAsync or just making it not use bind makes everything else work fine.

I think this is actually a case where some of the weird cross-component reactivity problems in Svelte 4 are fixed, and the previous "correct" behavior is only correct because Svelte 4 was losing the reactivity chain at some point. So here we just need to not bind:value on the optionsAsync SelectField, or perhaps bind to a different value variable.

dimfeld avatar Jul 31 '24 05:07 dimfeld

Duration problem is because getDelay tries to access $timer but it's also called before timer is actually created, in the initial call to timerStore. I think in Svelte 4 the variable declaration for timer might have been hoisted but it looks like it doesn't work that way anymore, just $timer is hoisted which now uses accessors to access the value inside the store.

This fixes it although it's a bit of a hack. There's probably a better way to write this but the standard tricks like typeof timer === 'undefined' didn't work.

<script lang="ts">
+  function getDelay(useTimer = true) {
-  function getDelay() {
+    const endTime = end ?? (useTimer ? $timer : null);
+    const newDuration = getDuration(start, endTime, duration);
-    const newDuration = getDuration(start, end ?? $timer, duration);

    // snip
  }

  const timer = timerStore({
+    delay: getDelay(false),
-    delay: getDelay(),
    disabled: end != null,
    onTick: () => {
      // Update delay based on display duration
      const newDelay = getDelay();
      if (newDelay != timer.getDelay()) {
        timer.setDelay(newDelay);
      }
    },
  });

dimfeld avatar Jul 31 '24 05:07 dimfeld

MultiSelect issues are related to mode = "immediate".

The change event handler in the demo page is called after the await point in applyChange, which means that it's effectively not part of the "what's happening in this reactive statement" tracking, and it sets value which then triggers an infinite reactive loop by calling applyChange again.

I'm not 100% sure of the full dependency cycle here, or why it breaks in Svelte 5 but worked on 4, but my current best fix is to wrap the call to applyChange in a setTimeout.

  $: if (mode === 'immediate' && $selection) {
    setTimeout(applyChange);
  }

dimfeld avatar Jul 31 '24 07:07 dimfeld

Just spotted another Svelte 5 regression with SelectField while looking at the docs. Sometimes (but not always) there is an extra empty option at the top.

image

techniq avatar Jul 31 '24 18:07 techniq

Did a quick investigation and it looks to go aways unless you add the "append slot (field actions)" example (along with basic). Having only those 2 examples causes the issue). Probably because the Form on:change is being called more than expected.

<h2>`append` slot (field actions)</h2>

<Preview>
  <Toggle let:on={open} let:toggle let:toggleOff>
    <SelectField {options}>
      <span slot="append" on:click|stopPropagation role="none">
        <Button icon={mdiPlus} class="text-surface-content/50 p-2" on:click={toggle} />
      </span>
    </SelectField>
    <Form
      initial={newOption()}
      on:change={(e) => {
        options = [e.detail, ...options];
      }}
      let:draft
      let:commit
      let:revert
    >
      <Dialog {open} on:close={toggleOff}>
        <div slot="title">Create new option</div>
        <div class="px-6 py-3 w-96 grid gap-2">
          <TextField
            label="Label"
            value={draft.label}
            on:change={(e) => {
              draft.label = e.detail.value;
            }}
            autofocus
          />
          <TextField
            label="Value"
            value={draft.value}
            on:change={(e) => {
              draft.value = e.detail.value;
            }}
          />
        </div>
        <div slot="actions">
          <Button on:click={() => commit()} color="primary">Add option</Button>
          <Button on:click={() => revert()}>Cancel</Button>
        </div>
      </Dialog>
    </Form>
  </Toggle>
</Preview>

Definitely more to look into, but this helps to reduce it down.

techniq avatar Jul 31 '24 18:07 techniq

So the root issue of the extra SelectField option being added is because <Form on:change={...}> is dispatched immediately in Svelte 5.

This is because the reactive dispatch in Form is called immediately in Svelte 5 (REPL) but not in Svelte 4 (REPL).

Component.svelte

<script>
  import { createEventDispatcher } from 'svelte';

  const dispatch = createEventDispatcher();
  let value = 0;	
  
  $: dispatch('change', { value });
</script>

App.svelte

<script>
  import Component from './Component.svelte';
</script>

<Component on:change={e => console.log('change', e.detail)} />

I'm debating on whether this is a reportable regression, or works as expected ($: console.log(value) runs immediately in Svelte 4, just not $: dispatch(value)).

If not, I need to come up with a solution to wait for the value to change. I initially thought of...

<script>
  //...

  let isMounted = false;
  onMount(() => {
    isMounted = true;
  });

  $: if (isMounted) {
    dispatch('change', $_state);
  }
</script>

...but then isMounted is updated reactively, which fires the change event.

@dimfeld thoughts?

techniq avatar Jul 31 '24 22:07 techniq

One workaround is to use a changeStore since the form state is a store in this case.

$: dispatch('change', $_state);

becomes...

const changed = changeStore(_state, (value) => dispatch('change', value.current));
$changed; // must subscribe to store to get onChange callbacks

techniq avatar Jul 31 '24 22:07 techniq

@dimfeld I decided to go ahead and use the changeStore fix/workaround. Still curious your thoughts if this change is work a report.

techniq avatar Jul 31 '24 22:07 techniq

It sounds more like a bug that it wasn't doing this in Svelte 4 :) And actually this is https://github.com/sveltejs/svelte/issues/4470

Yeah anything that suppresses that first firing seems fine.

dimfeld avatar Jul 31 '24 22:07 dimfeld

Using SelectField outside a dialog gives me the following warning in [email protected]. I'm not sure what's causing this, but the select field still seems to work fine until I add another select field, it will break the page because the browser can't repair broken HTML. This results in the error: Uncaught (in promise) HierarchyRequestError: Failed to execute 'appendChild' on 'Node': This node type does not support this method.

Warning,

(index):3922 Placing %sveltekit.body% directly inside <body> is not recommended,
as your app may break for users who have certain browser extensions installed.

Consider wrapping it in an element:

<div style="display: contents">
  %sveltekit.body%
</div>

iniznet avatar Oct 03 '24 19:10 iniznet

@iniznet That's rather interesting, and not sure what would be causing that off the top of my head. I first thought it might be because of the portal action, but SelectField/Menu/Popover doesn't use the action (only Dialog/Drawer/Backdrop).

techniq avatar Oct 03 '24 20:10 techniq

Closing this overarching issue as last remaining (known) regression (MultiSelect mode="immediate" freezes browser) has been fixed.

All that remains is re-adding API docs, which is being tracking in #435 and will be re-added once the library has been migrated to Svelte 5 (planning for EoY).

techniq avatar Nov 03 '24 03:11 techniq