skeletonlabs / skeleton

A complete design system and component solution, built on Tailwind.
https://skeleton.dev
MIT License
4.97k stars 316 forks source link

Svelte v5 RC: InputChip component produces errors #2711

Closed multiwebinc closed 4 months ago

multiwebinc commented 4 months ago

Current Behavior

I have a weird error with the InputChip. It doesn't work when it is a child of a <div> and a child of a <form>.

Note: This error occurs only in Svelte 5 RC, not in Svelte 4.

This works ✅:

<script>
    import { InputChip } from '@skeletonlabs/skeleton';
    let list = ['foo', 'bar', 'fizz', 'buzz'];
</script>

<InputChip bind:value={list} name="chips" />

This works ✅:

<script>
    import { InputChip } from '@skeletonlabs/skeleton';
    let list = ['foo', 'bar', 'fizz', 'buzz'];
</script>

<div>
  <InputChip bind:value={list} name="chips" />
</div>

This works ✅:

<script>
    import { InputChip } from '@skeletonlabs/skeleton';
    let list = ['foo', 'bar', 'fizz', 'buzz'];
</script>

<form>
  <InputChip bind:value={list} name="chips" />
</form>

This does not work ❌:

<script>
    import { InputChip } from '@skeletonlabs/skeleton';
    let list = ['foo', 'bar', 'fizz', 'buzz'];
</script>

<form>
  <div>
    <InputChip bind:value={list} name="chips" />
  </div>
</form>

I get the following errors:

client.js?v=7c866b2e:299 Uncaught (in promise) Svelte error: svelte_component_invalid_this_value
The `this={...}` property of a `<svelte:component>` must be a Svelte component, if defined

    in root.svelte

    at svelte_component_invalid_this_value (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:385:19)
    at Module.validate_dynamic_component (http://localhost:5174/node_modules/.vite/deps/chunk-ZC2UNLWN.js?v=7c866b2e:2813:7)
    at http://localhost:5174/.svelte-kit/generated/root.svelte:74:7
    at http://localhost:5174/node_modules/.vite/deps/chunk-ZC2UNLWN.js?v=7c866b2e:1035:30
    at execute_reaction_fn (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:1609:29)
    at execute_effect (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:1733:20)
    at create_effect (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:488:7)
    at branch (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:594:10)
    at http://localhost:5174/node_modules/.vite/deps/chunk-ZC2UNLWN.js?v=7c866b2e:1035:17
    at execute_reaction_fn (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:1609:29)Svelte error: svelte_component_invalid_this_value
The `this={...}` property of a `<svelte:component>` must be a Svelte component, if defined

    in root.svelte

    at svelte_component_invalid_this_value (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:385:19)
    at Module.validate_dynamic_component (http://localhost:5174/node_modules/.vite/deps/chunk-ZC2UNLWN.js?v=7c866b2e:2813:7)
    at http://localhost:5174/.svelte-kit/generated/root.svelte:74:7
    at http://localhost:5174/node_modules/.vite/deps/chunk-ZC2UNLWN.js?v=7c866b2e:1035:30
    at execute_reaction_fn (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:1609:29)
    at execute_effect (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:1733:20)
    at create_effect (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:488:7)
    at branch (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:594:10)
    at http://localhost:5174/node_modules/.vite/deps/chunk-ZC2UNLWN.js?v=7c866b2e:1035:17
    at execute_reaction_fn (http://localhost:5174/node_modules/.vite/deps/chunk-MVFJIXHO.js?v=7c866b2e:1609:29)
chunk-ZC2UNLWN.js?v=7c866b2e:1202 Uncaught TypeError: dom.getAttribute is not a function
    at Array.remove_defaults (chunk-ZC2UNLWN.js?v=7c866b2e:1202:25)
    at run_all (chunk-DKI5ZJQF.js?v=7c866b2e:12:11)
    at process_idle_tasks (chunk-MVFJIXHO.js?v=7c866b2e:1178:3)

Expected Behavior

No response

Steps To Reproduce

No response

Link to Reproduction / Stackblitz

https://stackblitz.com/edit/vitejs-vite-ijoavm?file=src%2Froutes%2F%2Blayout.svelte&terminal=dev

More Information

No response

endigo9740 commented 4 months ago

UPDATE: see my next post for an update - I was testing on Svelte 4 here


Hmm, so this is an interesting one.

I'm having trouble reproducing the error as stated though, even with your reproduction. The only error logged is that the root layout doesn't contain a <slot /> pass through. After resolving that I able to refresh and use the InputChip as expected though.

Screenshot 2024-06-05 at 3 08 48 PM

Putting that aside, I've generated a new local project using the Skeleton CLI, inserted the InputChips per the first example in the documentation and tested by itself, with a wrapping div, and with a wrapping form. The only notable difference is I did this within the home page rather than root layout. All worked as expected.

The thing that jumps out to me here is the error though:

client.js?v=7c866b2e:299 Uncaught (in promise) Svelte error: svelte_component_invalid_this_value
The `this={...}` property of a `<svelte:component>` must be a Svelte component, if defined

It's stating a failure because of an unbinded (unbound?) <svelte:component>. But the InputChip does not make use of this feature. You can check for yourself:

https://github.com/skeletonlabs/skeleton/blob/master/packages/skeleton/src/lib/components/InputChip/InputChip.svelte

I might encourage you to verify if you're using <svelte:component> anywhere in your project and if so, make sure it's properly defined per the documentation:

https://svelte.dev/docs/special-elements#svelte-component

endigo9740 commented 4 months ago

@multiwebinc I just realized you put Svelte 5 RC in the title. Give me a moment and I'll retest and post my findings...

UPDATE

I can confirm I was able to replicate after migrating to the Svelte 5 RC:

Screenshot 2024-06-05 at 3 24 17 PM

I'm still investigating the cause, but this is very odd as we don't use this feature in the InputChip component.

UPDATE 2

I've also confirmed that replacing the div with seemingly any other HTML element provides the same results: section, fieldset, etc.

endigo9740 commented 4 months ago

@multiwebinc see my updates above. I'm at a bit of a loss here, so I went ahead and pinged a couple folks on the Skeleton team that are a bit more knowledgable about Svelte 5. They may chime with thoughts, suggestions, or additional requests from you.

In the meantime, the only work around I can think of is to avoid nesting the InputChip in this manner. Consider adding styles directly to the component itself using the class attribute if you need to.

I will go ahead and point out the obvious here - that Svelte 5 is not yet ready for production use. So this is not completely unexpected: https://svelte-5-preview.vercel.app/status

Skeleton is at the mercy of any and all breaking changes and bugs until they hit a stable release. The fact that it's reporting an error that seems to be incorrect makes me think this may be a bug on their end though. If so, we'll pass it forward.

In the meantime, thanks for your patience and for reporting the issue!

multiwebinc commented 4 months ago

@endigo9740 I just took a quick look at the component. This is just a guess, but the component contains a <form> element. Therefore, using this component in a <form> element creates invalid HTML.

Warning: It's strictly forbidden to nest a form inside another form. Nesting can cause forms to behave unpredictably, so it is a bad idea. (source)

Hugos68 commented 4 months ago

@multiwebinc You are dead on! After some debugging I came to the same conclusion.

REPL Reproduction

As you can see, bind:value inside a nested form causes a compiler error. This is still not correct behaviour from Svelte though. Although it's not valid HTML dead on erroring seems odd, I'm still going to report it to the Svelte team but we (the skeleton team) are indeed in the wrong here for using a <form> internally and allowing the user to use it externally.

I've started a discussion on discord here

multiwebinc commented 4 months ago

@Hugos68 I tried removing the <form> and added an on:keypress handler to the <input> and it works just fine, however I'm not sure how this would work on mobile.

(Note: The imports are messed up in this)

<script lang="ts" context="module">
    import { fly, scale } from 'svelte/transition';
    // import { type Transition, type TransitionParams, prefersReducedMotionStore } from '../../index.js';
    import { dynamicTransition } from '../../../../../../node_modules/@skeletonlabs/skeleton/dist/internal/transitions.js';

    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    type FlyTransition = typeof fly;
    type ListTransitionIn = Transition;
    type ListTransitionOut = Transition;

    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    type ScaleTransition = typeof scale;
    type ChipTransitionIn = Transition;
    type ChipTransitionOut = Transition;
</script>

<script
    lang="ts"
    generics="ListTransitionIn extends Transition = FlyTransition, ListTransitionOut extends Transition = FlyTransition, ChipTransitionIn extends Transition = ScaleTransition, ChipTransitionOut extends Transition = ScaleTransition"
>
    import { createEventDispatcher, onMount } from 'svelte';
    import { flip } from 'svelte/animate';
    import {
        type Transition,
        type CssClasses,
        type TransitionParams,
        type SvelteEvent,
        prefersReducedMotionStore
    } from '@skeletonlabs/skeleton';

    // Types
    // import type { CssClasses, SvelteEvent } from '../../index.js';

    // Event Dispatcher
    type InputChipEvent = {
        add: { event: KeyboardEvent; chipIndex: number; chipValue: string };
        remove: { event: MouseEvent; chipIndex: number; chipValue: string };
        addManually: { chipIndex: number; chipValue: string };
        removeManually: { chipValue: string };
        invalid: { event: KeyboardEvent; input: string };
        invalidManually: { input: string };
    };
    const dispatch = createEventDispatcher<InputChipEvent>();

    // Props
    /** Bind the input value. */
    export let input = '';
    /**
     * Set a unique select input name.
     * @type {string}
     */
    export let name: string;
    /** An array of values. */
    export let value: any[] = [];
    /**
     * Provide a whitelist of accepted values.
     * @type {string[]}
     */
    export let whitelist: string[] = [];
    /** Maximum number of chips. Set -1 to disable. */
    export let max = -1;
    /** Set the minimum character length. */
    export let minlength = -1;
    /** Set the maximum character length. */
    export let maxlength = -1;
    /** When enabled, allows for uppercase values. */
    export let allowUpperCase = false;
    /** When enabled, allows for duplicate values. */
    export let allowDuplicates = false;
    /**
     * Provide a custom validator function.
     * @type {function}
     */
    export let validation: (...args: any[]) => boolean = () => true;

    /** The duration of the flip (first, last, invert, play) animation. */
    export let duration = 150;
    /** Set the required state for this input field. */
    export let required = false;

    // Props (styles)
    /** Provide classes or a variant to style the chips. */
    export let chips: CssClasses = 'variant-filled';
    /** Provide classes used to indicate invalid state. */
    export let invalid: CssClasses = 'input-error';
    /** Provide classes to set padding styles. */
    export let padding: CssClasses = 'p-2';
    /** Provide classes to set border radius styles. */
    export let rounded: CssClasses = 'rounded-container-token';

    // Props (regions)
    /** Provide arbitrary classes to style the chip wrapper region. */
    export let regionChipWrapper: CssClasses = '';
    /** Provide arbitrary classes to style the chip list region. */
    export let regionChipList: CssClasses = '';
    /** Provide arbitrary classes to style the input field region. */
    export let regionInput: CssClasses = '';

    // Props (A11y)
    /** Provide the ARIA label for the select input. */
    export let label = 'Chips select';

    // Props (transition)
    /**
     * Enable/Disable transitions
     * @type {boolean}
     */
    export let transitions = !$prefersReducedMotionStore;
    /**
     * Provide the transition used in list on entry.
     * @type {ListTransitionIn}
     */
    export let listTransitionIn: ListTransitionIn = fly as ListTransitionIn;
    /**
     * Transition params provided to `ListTransitionIn`.
     * @type {TransitionParams}
     */
    export let listTransitionInParams: TransitionParams<ListTransitionIn> = {
        duration: 150,
        opacity: 0,
        y: -20
    };
    /**
     * Provide the transition used in list on exit.
     * @type {ListTransitionOut}
     */
    export let listTransitionOut: ListTransitionOut = fly as ListTransitionOut;
    /**
     * Transition params provided to `ListTransitionOut`.
     * @type {TransitionParams}
     */
    export let listTransitionOutParams: TransitionParams<ListTransitionOut> = {
        duration: 150,
        opacity: 0,
        y: -20
    };
    /**
     * Provide the transition used in chip on entry.
     * @type {ChipTransitionIn}
     */
    export let chipTransitionIn: ChipTransitionIn = scale as ChipTransitionIn;
    /**
     * Transition params provided to `ChipTransitionIn`.
     * @type {TransitionParams}
     */
    export let chipTransitionInParams: TransitionParams<ChipTransitionIn> = {
        duration: 150,
        opacity: 0
    };
    /**
     * Provide the transition used in chip on exit.
     * @type {ChipTransitionOut}
     */
    export let chipTransitionOut: ChipTransitionOut = scale as ChipTransitionOut;
    /**
     * Transition params provided to `ChipTransitionOut`.
     * @type {TransitionParams}
     */
    export let chipTransitionOutParams: TransitionParams<ChipTransitionOut> = {
        duration: 150,
        opacity: 0
    };

    // Classes
    const cBase = 'textarea cursor-pointer';
    const cChipWrapper = 'space-y-4';
    const cChipList = 'flex flex-wrap gap-2';
    const cInputField = 'unstyled bg-transparent border-0 !ring-0 p-0 w-full';

    // Local
    let inputValid = true;
    let chipValues: Array<{ val: (typeof value)[0]; id: number }> =
        value?.map((val) => {
            return { val: val, id: Math.random() };
        }) || [];

    // Reset Form
    // function resetFormHandler() {
    //  value = [];
    // }
    let selectElement: HTMLSelectElement;
    // onMount(() => {
    //  // Verify external form is present
    //  if (!selectElement.form) return;

    //  const externalForm = selectElement.form as HTMLFormElement;

    //  // Attach reset event listener to external form
    //  externalForm.addEventListener('reset', resetFormHandler);

    //  // Return onDestroy handler that will remove the event listener from the external form
    //  return () => {
    //      externalForm.removeEventListener('reset', resetFormHandler);
    //  };
    // });

    function onInputHandler(): void {
        inputValid = true;
    }

    function validateCustom(chip: string) {
        return validation === undefined || validation(chip);
    }

    function validateCount() {
        return max === -1 || value.length < max;
    }

    function validateLength(chip: string) {
        return (
            (minlength === -1 || chip.length >= minlength) &&
            (maxlength === -1 || chip.length <= maxlength)
        );
    }

    function validateWhiteList(chip: string) {
        return whitelist.length === 0 || whitelist.includes(chip);
    }

    function validateDuplicates(chip: string) {
        return allowDuplicates || !value.includes(chip);
    }

    function validate(chip = ''): boolean {
        if (!chip && !input) return false;
        // Format: trim value
        chip = chip !== '' ? chip.trim() : input.trim();
        return (
            validateCustom(chip) &&
            validateCount() &&
            validateLength(chip) &&
            validateWhiteList(chip) &&
            validateDuplicates(chip)
        );
    }

    function addChipCommon(chip: string) {
        // Format: to lowercase (if enabled)
        chip = allowUpperCase ? chip : chip.toLowerCase();
        // Append value to array
        value.push(chip);
        value = value;
        chipValues.push({ val: chip, id: Math.random() });
        chipValues = chipValues;
    }

    function removeChipCommon(chip: string) {
        let chipIndex = value.indexOf(chip);
        // Remove value from array
        value.splice(chipIndex, 1);
        value = value;
        chipValues.splice(chipIndex, 1);
        chipValues = chipValues;
    }

    function addChipInternally(event: SvelteEvent<KeyboardEvent>): void {
        if (event.key === 'Enter' || event.keyCode === 13) {
            event.preventDefault();
            // Validate
            inputValid = validate();
            // When the onInvalid hook is present
            if (inputValid === false) {
                /** @event {{ event: Event, input: any  }} invalid - Fires when the input value is invalid. */
                dispatch('invalid', { event, input });
                return;
            }
            addChipCommon(input);
            /** @event {{ event: Event, chipIndex: number, chipValue: string }} add - Fires when a chip is added. */
            dispatch('add', { event, chipIndex: value.length - 1, chipValue: input });
            // Clear input value
            input = '';
        }
    }

    function removeChipInternally(
        event: SvelteEvent<MouseEvent, HTMLButtonElement>,
        chipIndex: number,
        chipValue: string
    ): void {
        if ($$restProps.disabled) return;
        removeChipCommon(chipValue);
        /** @event {{ event: Event, chipIndex: number, chipValue: string }} remove - Fires when a chip is removed. */
        dispatch('remove', { event, chipIndex, chipValue });
    }

    // Export functions
    export function addChip(chip: string) {
        // Validate
        inputValid = validate(chip);
        // When the onInvalid hook is present
        if (inputValid === false) {
            /** @event {{ input: string  }} invalidManually - Fires when the manually added value is invalid. */
            dispatch('invalidManually', { input: chip });
            return;
        }
        addChipCommon(chip);
        /** @event {{ chipIndex: number, chipValue: string }} addManually - Fires when a chip is added manually. */
        dispatch('addManually', { chipIndex: value.length - 1, chipValue: chip });
    }

    export function removeChip(chip: string) {
        if ($$restProps.disabled) return;
        removeChipCommon(chip);
        /** @event {{ chipValue: string }} removeManually - Fires when a chip is removed manually. */
        dispatch('removeManually', { chipValue: chip });
    }

    // State
    $: classesInvalid = inputValid === false ? invalid : '';
    // Reactive
    $: classesBase = `${cBase} ${padding} ${rounded} ${$$props.class ?? ''} ${classesInvalid}`;
    $: classesChipWrapper = `${cChipWrapper} ${regionChipWrapper}`;
    $: classesChipList = `${cChipList} ${regionChipList}`;
    $: classesInput = `${cInputField} ${regionInput}`;
    $: chipValues =
        value?.map((val, i) => {
            if (chipValues[i]?.val === val) return chipValues[i];
            return { id: Math.random(), val: val };
        }) || [];
</script>

<div class="input-chip {classesBase}" class:opacity-50={$$restProps.disabled}>
    <!-- NOTE: Don't use `hidden` as it prevents `required` from operating -->
    <div class="h-0 overflow-hidden">
        <select
            bind:this={selectElement}
            bind:value
            {name}
            multiple
            {required}
            aria-label={label}
            tabindex="-1"
        >
            <!-- NOTE: options are required! -->
            {#each value as option}<option value={option}>{option}</option>{/each}
        </select>
    </div>
    <!-- Chip Wrapper -->
    <div class="input-chip-wrapper {classesChipWrapper}">
        <!-- Input Field -->
        <!-- <form on:submit={addChipInternally}> -->
        <input
            type="text"
            bind:value={input}
            placeholder={$$restProps.placeholder ?? 'Enter values...'}
            class="input-chip-field {classesInput}"
            on:input={onInputHandler}
            on:input
            on:focus
            on:blur
            disabled={$$restProps.disabled}
            on:keypress={(e) => addChipInternally(e)}
        />
        <!-- </form> -->
        <!-- Chip List -->
        {#if chipValues.length}
            <div
                class="input-chip-list {classesChipList}"
                in:dynamicTransition|local={{
                    transition: listTransitionIn,
                    params: listTransitionInParams,
                    enabled: transitions
                }}
                out:dynamicTransition|local={{
                    transition: listTransitionOut,
                    params: listTransitionOutParams,
                    enabled: transitions
                }}
            >
                {#each chipValues as { id, val }, i (id)}
                    <!-- Wrapping div required for FLIP animation -->
                    <div animate:flip={{ duration }}>
                        <button
                            type="button"
                            class="chip {chips}"
                            on:click={(e) => removeChipInternally(e, i, val)}
                            on:click
                            on:keypress
                            on:keydown
                            on:keyup
                            in:dynamicTransition|local={{
                                transition: chipTransitionIn,
                                params: chipTransitionInParams,
                                enabled: transitions
                            }}
                            out:dynamicTransition|local={{
                                transition: chipTransitionOut,
                                params: chipTransitionOutParams,
                                enabled: transitions
                            }}
                        >
                            <span>{val}</span>
                            <span>✕</span>
                        </button>
                    </div>
                {/each}
            </div>
        {/if}
    </div>
</div>
endigo9740 commented 4 months ago

@multiwebinc ahh yeah that would definitely cause issues. I'm aware of a few issues like this in our components, many of which should be addressed during the rewrite for v3. In the meantime, we can aim to prioritize a v2 patch based on your suggestion. I do believe this was added due to mobile support, but it was a short-sighted choice and not the way should be handled.

endigo9740 commented 4 months ago

@multiwebinc I've created a PR with a potential fix.

Capturing the Enter key can be finicky on mobile, so I've reached out to our community on Discord to try testing this change on a wider range of mobile devices and browsers. We want to avoid regressions! If you could, please open this Preview URL in every mobile phone you have handy to verify all is well:

https://skeleton-docs-p5zez2645-skeleton-labs.vercel.app/components/input-chips

Expectations:

  1. Open the page
  2. Enter text into any of the InputChip examples
  3. Tap Enter on your virtual keyboard to submit
  4. A new chip should be added

Assuming this is working universally, we'll merge and this will be part of the next v2 release in about a week and a half.

multiwebinc commented 4 months ago

@endigo9740 I tried with Opera, Chrome, Samsung browser and Firefox on Android and all work as expected.