bestguy / sveltestrap

Bootstrap 4 & 5 components for Svelte
https://sveltestrap.js.org
MIT License
1.3k stars 183 forks source link

change on:change and bind order in components #461

Open shaun-wild opened 2 years ago

shaun-wild commented 2 years ago

In the Input component, the order of the on:xxx and bind:xxx should be swapped, because the values will not be up to date when they are read in the on:xxx listener.

Example REPL: https://svelte.dev/repl/3b540047caa44fc6b0e0c7ad5c23e3f7?version=3.46.4

Apparently, the order of these attributes matters!

shaun-wild commented 2 years ago

I found a workaround for anyone interested.

You can add

await new Promise(setTimeout)

At the top of the on:xxxx handler.

clemens-tolboom commented 2 years ago

I ran into this too using <Input> but making it a bare <select> and take care of the order it worked.

<select
            bind:value={voice}
            on:change={(ev) => doSpeak(ev)}
        >

Testing with the REPL which uses <select> I do see swapping order to break or make it work correctly.

But this is a svelte bug instead I guess.

clemens-tolboom commented 2 years ago

This is a know 'bug' https://github.com/sveltejs/svelte/issues/4616 but it seems to be a feature and has a merged PR https://github.com/sveltejs/svelte/pull/6887 documenting it.

See just above https://svelte.dev/docs#template-syntax-element-directives-bind-property-binding-select-value

So this is a bug in sveltestrap :-O

clemens-tolboom commented 2 years ago

A quick scan of the code shows more problems as bind: must be before on: unless ... see links above.

FormCheck.svelte
41-      on:change
42-      on:focus
43-      on:input
44:      bind:group
45:      bind:this={inner}
46-      {disabled}
47-      {name}
48-      {value}
--
57-      on:change
58-      on:focus
59-      on:input
60:      bind:checked
61:      bind:this={inner}
62-      {disabled}
63-      {name}
64-      {value}
--
73-      on:change
74-      on:focus
75-      on:input
76:      bind:checked
77:      bind:this={inner}
78-      {disabled}
79-      {name}
80-      {value}

Input

Input.svelte
103-      on:keydown
104-      on:keypress
105-      on:keyup
106:      bind:value
107:      bind:this={inner}
108-      {disabled}
109-      {name}
110-      {placeholder}
--
123-      on:keydown
124-      on:keypress
125-      on:keyup
126:      bind:value
127:      bind:this={inner}
128-      {disabled}
129-      {name}
130-      {placeholder}
--
143-      on:keydown
144-      on:keypress
145-      on:keyup
146:      bind:value
147:      bind:this={inner}
148-      {disabled}
149-      {name}
150-      {placeholder}
--
162-      on:keydown
163-      on:keypress
164-      on:keyup
165:      bind:value
166:      bind:this={inner}
167-      {disabled}
168-      {multiple}
169-      {name}
--
183-      on:keydown
184-      on:keypress
185-      on:keyup
186:      bind:files
187:      bind:value
188:      bind:this={inner}
189-      {disabled}
190-      {invalid}
191-      {multiple}
--
207-      on:keydown
208-      on:keypress
209-      on:keyup
210:      bind:checked
211:      bind:inner
212:      bind:group
213:      bind:value
214-      {disabled}
215-      {invalid}
216-      {label}
--
231-      on:keydown
232-      on:keypress
233-      on:keyup
234:      bind:value
235:      bind:this={inner}
236-      {disabled}
237-      {name}
238-      {placeholder}
--
251-      on:keydown
252-      on:keypress
253-      on:keyup
254:      bind:value
255:      bind:this={inner}
256-      {readonly}
257-      {name}
258-      {disabled}
--
270-      on:keydown
271-      on:keypress
272-      on:keyup
273:      bind:value
274:      bind:this={inner}
275-      {disabled}
276-      {name}
277-      {placeholder}
--
289-      on:keydown
290-      on:keypress
291-      on:keyup
292:      bind:value
293:      bind:this={inner}
294-      {disabled}
295-      {name}
296-      {placeholder}
--
307-      on:keydown
308-      on:keypress
309-      on:keyup
310:      bind:value
311:      bind:this={inner}
312-      {readonly}
313-      class={classes}
314-      {name}
--
327-      on:keydown
328-      on:keypress
329-      on:keyup
330:      bind:value
331:      bind:this={inner}
332-      {disabled}
333-      {name}
334-      {placeholder}
--
346-      on:keydown
347-      on:keypress
348-      on:keyup
349:      bind:value
350:      bind:this={inner}
351-      {disabled}
352-      {name}
353-      {placeholder}
--
364-      on:keydown
365-      on:keypress
366-      on:keyup
367:      bind:value
368:      bind:this={inner}
369-      {readonly}
370-      class={classes}
371-      {name}
--
383-      on:keydown
384-      on:keypress
385-      on:keyup
386:      bind:value
387:      bind:this={inner}
388-      {readonly}
389-      class={classes}
390-      {name}
--
403-      on:keydown
404-      on:keypress
405-      on:keyup
406:      bind:value
407:      bind:this={inner}
408-      {disabled}
409-      {name}
410-      {placeholder}
--
423-      on:keydown
424-      on:keypress
425-      on:keyup
426:      bind:value
427:      bind:this={inner}
428-      {disabled}
429-      {name}
430-      {placeholder}
--
443-      on:keydown
444-      on:keypress
445-      on:keyup
446:      bind:value
447:      bind:this={inner}
448-      {disabled}
449-      {name}
450-      {placeholder}
--
480-    on:keydown
481-    on:keypress
482-    on:keyup
483:    bind:value
484:    bind:this={inner}
485-    {disabled}
486-    {name}
487-    {placeholder}
--
491-  <select
492-    {...$$restProps}
493-    class={classes}
494:    bind:value
495:    bind:this={inner}
496-    on:blur
497-    on:change
498-    on:focus
--
513-    on:focus
514-    on:change
515-    on:input
516:    bind:value
517:    bind:this={inner}
518-    {name}
519-    {disabled}>
520-    <slot />
shaun-wild commented 2 years ago

This is what I said in my original post.

I'm not sure if this is a bug or not, but it doesn't seem very intuitive nonetheless.

I hope my workaround helps.

clemens-tolboom commented 2 years ago

My workaround was use plain <select>.

@Shaunwild97 is my PR #468 ok? Maybe comment on it?

clemens-tolboom commented 2 years ago

I can confirm this happen too on checkbox. Order matters.

        <Input
            type="checkbox"
            label="Use X"
            bind:checked={isChecked}
            on:change={(ev) => console.log(isChecked}
        />

Guess we could wait on the maintainer reviewing the PR and this issue.

shaun-wild commented 2 years ago

@clemens-tolboom Did you see my comment on your PR?

https://github.com/bestguy/sveltestrap/pull/468#issuecomment-1082432078

clemens-tolboom commented 2 years ago

I somehow missed the plural form in components.

I still don't like this https://github.com/sveltejs/svelte/issues/4616 feature ... not sure what the old value is good for ...

But it feels like we cannot escape this as in 'should we keep the user entered order' instead of dictate order by Sveltestrap? (is that even possible?)

MDesharnaisX commented 1 year ago

I'm bringing back this subject to life. As it is, the change event occurs before the bind:value. From what I have read, the issue is considered in svelte as a feature but in sveltestrap, the feature (choose your event ordering) becomes a liability because the order if fixed and in my opinion is fixed the wrong way. If it is not possible to choose the order, could it be changed ? IMHO, it is much more logic to have the value bound before the onchange then having to guess the new value or even worse calling a setTimeout to let the bind be done before executing your on:change.

rornic commented 1 year ago

+1 I've just run into the same issue with checkboxes.

alucryd commented 1 year ago

Just lost several hours to this bug... Is there anything preventing the pull request from being merged? Select and checkbox inputs are basically unusable as they are now.