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

Svelte 5 compatibility #144

Open
techniq opened this issue Nov 29, 2023 · 19 comments
Open

Svelte 5 compatibility #144

techniq opened this issue Nov 29, 2023 · 19 comments

Comments

@techniq
Copy link
Owner

techniq commented Nov 29, 2023

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: f419435


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

example: https://svelte5-docs.svelte-ux.pages.dev/docs/components/SelectField
fix: #448


✅ FIXED: Duration Cannot access 'timer' before initialization

example: https://svelte5-docs.svelte-ux.pages.dev/docs/components/Duration
fix: #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: sveltejs/svelte#12440, see also: sveltejs/svelte#12427 (comment)
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: #437
fix: add explicit document.body for example, likely only affects docs (presence of .PortalTarget element)

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

issue: sveltejs/svelte#12090
fix: add explilcit <tbody>


✅ FIXED: can only receive attributes, not directives

issue: sveltejs/svelte#10382
fix: sveltejs/svelte#10391


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

Fixed via: sveltejs/svelte#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: sveltejs/svelte#9710
fix: sveltejs/svelte#9727

@CropWatchDevelopment
Copy link

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!

@techniq
Copy link
Owner Author

techniq commented Nov 30, 2023

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.

@CropWatchDevelopment
Copy link

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!

@techniq
Copy link
Owner Author

techniq commented Nov 30, 2023

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.

@dimfeld
Copy link
Contributor

dimfeld commented Feb 3, 2024

Filed sveltejs/svelte#10382 because SelectField.svelte fails to compile with latest Svelte 5.

CleanShot 2024-02-02 at 20 42 27

@techniq
Copy link
Owner Author

techniq commented Feb 3, 2024

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.

@dimfeld
Copy link
Contributor

dimfeld commented Feb 5, 2024

Haven't tried it but I think this will hit svelte-ux as well sveltejs/svelte#10395

edit: this one is fixed now

@techniq
Copy link
Owner Author

techniq commented Jul 14, 2024

Identified the root cause of the Dialog/Drawer problem (portal'd content with elements in the same conditional) - sveltejs/svelte#12440

@techniq
Copy link
Owner Author

techniq commented Jul 15, 2024

Workaround identified - 54d9d39

@dimfeld
Copy link
Contributor

dimfeld commented Jul 31, 2024

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
Copy link
Contributor

dimfeld commented Jul 31, 2024

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
Copy link
Contributor

dimfeld commented Jul 31, 2024

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
Copy link
Contributor

dimfeld commented Jul 31, 2024

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);
  }

@techniq
Copy link
Owner Author

techniq commented Jul 31, 2024

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
Copy link
Owner Author

techniq commented Jul 31, 2024

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
Copy link
Owner Author

techniq commented Jul 31, 2024

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
Copy link
Owner Author

techniq commented Jul 31, 2024

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
Copy link
Owner Author

techniq commented Jul 31, 2024

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

@dimfeld
Copy link
Contributor

dimfeld commented Jul 31, 2024

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

Yeah anything that suppresses that first firing seems fine.

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

No branches or pull requests

3 participants