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

Two-way binding on:change and bind:value inconsistency depending on definition order #4616

Closed
rowanparker opened this issue Mar 31, 2020 · 10 comments

Comments

@rowanparker
Copy link

Describe the bug
If you define an on:change attribute before a bind:value attribute, the on:change handler doesn't work.

To Reproduce
This REPL shows how it should work.
https://svelte.dev/repl/b2dfff7844614d9e8ce07f8d41fc3086?version=3.20.1

Swap lines 44 and 45 around. The input boxes will stop updating when the select is changed.

Expected behavior
It shouldn't matter whether you define the event handler before the bind.

Severity
Maybe this is expected behaviour, but I couldn't see anything obvious in the docs to explain it.

@Conduitry Conduitry added the docs label Mar 31, 2020
@Conduitry
Copy link
Member

It is updating as far as I can tell, just based on the old value of the dropdown. If you place the on:change before the bind:value, your event handler will be attached (and called) before the internal event handler added by bind:value - and similarly if bind:value is before on:change then your change handler will see the updated bound value. This is expected behavior, but ought to be documented somewhere.

@antony
Copy link
Member

antony commented Mar 31, 2020

Also as an aside, your REPL is far, far too complicated. You've got a much better chance of us being able to see your issue and fix it if you boil it down to just the minimum required code in order to reproduce the issue.

@arxpoetica
Copy link
Member

I tried to whittle the example down to size: https://svelte.dev/repl/2dc3f7b5032a4049953777eacfb0bc69?version=3.20.1

@mindrones
Copy link
Member

I've documented this in #5166 but it got me thinking that it would probably be more intuitive if this wasn't dependent on the definition order, with Svelte attaching binding handlers before event handlers automatically.

@Conduitry
Copy link
Member

It's more powerful to have them happen in definition order, because this lets you intercept thing in event handlers or in actions.

@pngwn
Copy link
Member

pngwn commented Jul 18, 2020

Yeah, I think this was added a feature due to the limitations of a fixed order. I specifically remember @mrkishi having issues with the fixed order (although that was actions and bindings, rather than events), iirc.

@iluuu1994
Copy link

This issue cost me quite a bit of time. It really really should be mentioned in the docs, regardless of whether it's a feature or a bug.

@shaun-wild
Copy link

I do not like this feature; it is very confusing and the lack of documentation doesn't help.

I don't feel it is pragmatic for the order of attributes on a markup language to affect behavior, I think if the order in which the change event fires and the bind updates should be configurable, it should be elsewhere.

This has the added problem of making component libraries dictate the order of these attributes in their components, as there doesn't seem to be any way to allow users to specify the orders of attributes on sub-components.

@mrkishi
Copy link
Member

mrkishi commented Apr 1, 2022

What order should things be executed in, if not markup order? Should it be randomized to guarantee there's no defined order?
Or should we do bind: first by definition and not allow event handlers to ever have priority over bindings?

I think it's actually pragmatic for markup order to be respected:

<header>First!</header>
<p class="red" class="blue">What color am I?</p>

In this example, p is logically and unambiguously after header, so it should be no surprise markup order already matters for tags. As for attributes, would blue sometimes be applied? No, browsers also respect markup order and ignore duplicate attribute definitions, so it'll always be red.

Back to svelte:

<some-tag a={foo()} b={baz()} use:bar on:event={handle}/>

Would you expect foo and bar to be called in an undefined order? Would the event handler be set by the time the action is called?

If we just decided on an order like "bindings, then events, then actions, then attributes," we'd be preventing users from choosing the priority they actually need. For instance, folks usually treat actions as the closest-to-html you can get, so it wouldn't be unreasonable to expect you could stop event propagation from an action, but that wouldn't be possible in this cherry-picked order. If you switch it around, it could surprise people like you that an event executed before a binding updated.

But you said we just need to make it configurable somewhere. Alright... Where? Markup order is the simplest configuration there is to define an order. Besides, the order would need to be configurable per component. If it wasn't, we'd have components that needed a certain order but broke because the order changed around under their feet by user configuration.

This has the added problem of making component libraries dictate the order of these attributes in their components, as there doesn't seem to be any way to allow users to specify the orders of attributes on sub-components.

That sounds like a failure of that specific component's API. As explained above, app-wide configuration would mean no component library could rely on any specific order, so you'd fix this specific use-case while breaking others.

This was documented in #6887, but feel free to propose actionable improvements to the documentation.

@Conduitry
Copy link
Member

I agree that other solutions to this problem seem worse - and that running bindings, actions, and event handlers in the order that they are present on the element is the tidiest solution to this problem. It's also documented as of several months ago, so I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants