thisbeyond / solid-select

The Select component for Solid.
https://solid-select.com
MIT License
172 stars 18 forks source link

onChange called multiple times on initialize #65

Open robodna opened 2 months ago

robodna commented 2 months ago

Should setting initialValue cause the onChange event to be dispatched? I was expecting to be able to set the initial value without having onChange trigger. I'm currently trying to find out why onChange is getting called 3 times, the first time the onChange value is null, second and third times the values are the correct object.

Thanks,

martinpengellyphillips commented 2 months ago

initialValue is reactive so that you can change it externally. If you change it, then it makes sense to have onChange called in my view. The fact it happens on first set is a side effect of that decision.

I might look at some optimisations if it's producing duplicate calls on no change though.

robodna commented 2 months ago

Maybe it should behave like a native select element. From what I understand, onChange is only called when the user interacts with the element. ( onChange is for UI change only, but onInput triggers for non-user initiated changes )

I'll try to determine what is causing the double trigger ( if we ignore the first 'null' value onChange event )

Also, if the onChange triggeres, some forms may then be marked as 'dirty', even though the user did not dirty the form.

martinpengellyphillips commented 2 months ago

Note that the input event is not fired when JavaScript changes an element's value programmatically.

I've never particularly liked the different events approach of the native elements anyway 😄

I'll have a think. It might be reasonable to pass a reason parameter to onChange so the consumer can choose to ignore changes generated by initialValue change or not 🤔

robodna commented 1 month ago

I don't think adding a reason parameter will solve the issue. There needs to be a way to set the initial value programatically without triggering the onChange event. Many components like modularforms watch for onChange for inputs and expect an input event, and not a custom event as you suggest.

martinpengellyphillips commented 1 month ago

We're not emitting an event at all - onChange is a plain callback that passes the value not an event instance. So that's not a constraint here (you'd already have to do something custom to integrate with modularforms today).

There are many different approaches to forms and I don't intend to make solid-select integrate directly with a form (e.g. I don't have a hidden select etc under the hood). I prefer being explicit here with consumers doing their own form integration - for example, in my own apps I do <Select onChange={field.setValue} ... />

If you want to play with customising the behaviour though, take a look at the Select component - here is where the initialValue logic is handled.

robodna commented 1 month ago

Ok. To confirm, there is not way to set the initial value without triggering the onChange event? Also, the onChange event for solid select has different behavior than native select onChange event? Knowing this will help me find out why onChange is getting called 3 times when the form is first rendered, before the user has touched it.

For modularforms, if you use field.setValue, does it work? I though SolidSelect passed one or more objects to the onChange callback, and modularforms would not know what to do with that object. ( modularform field has number or string types but not an object )

Also, how do you prevent modularforms from showing the form as 'dirty' as soon as it is populated, before the user has touched it?

I plan on creating a 'wrapper' element which will dispatch an input event and de-bounce the duplicate onChange triggers if needed.

robodna commented 1 month ago

I just tested native select and input elements. The onInput and onChange events only dispatch on user interaction. Setting initial values does not dispatch those events. It would make sense for SelectSolid to have the same behavior if it is to be used as a form element. If it was not designed to be used in forms, I will just create a wrapper element.

martinpengellyphillips commented 1 month ago

Ok. To confirm, there is not way to set the initial value without triggering the onChange event? Also, the onChange event for solid select has different behavior than native select onChange event?

Yes and yes to the first points. Some details: the low level primitive createSelect can be used to build your own Select component and it does not trigger onChange on initialisation. The builtin Select component is then an opinionated implementation using that primitive and it currently does result in onChange on initialisation. In all cases, no native dom event is being raised by solid-select. It passes raw values around instead.

I don't use modularforms myself so I can't comment on how to integrate with it (I use my own custom form library that I build early on with SolidJS). Perhaps ask the modularforms folks how they support integrating with custom components that don't use native events.

For reference, in my own form logic I tend to use a blur event to determine if field is 'dirty'/'touched' rather than rely on the value being set.

robodna commented 1 month ago

Thanks for the additional info. I guess it comes down to how the element is initialized. Personally, I envision initializing an input element in the same way as initializing a variable or setting an initial state before state changes are tracked. I think SolidJS works in the same way when creating a signal or store. ( I could be wrong, but I don't think they trigger a change notification to subscribers when they are first initialized )

There is also onUpdate in SolidJS... part of my confusion with SolidSelect was that onChange is the same event name as native select elements. If it was named onUpdate, maybe it would have prevented that confusion on my part.

Also, I could just wrap SolidSelect in an element, and add a hidden input to get what I envision.

martinpengellyphillips commented 1 month ago

Signals in SolidJS set up the subscription on access, so they do effectively call with the initialised value (but perhaps not in the way you are thinking.

There is also onUpdate in SolidJS

I'm not familiar with this - can you share the link.

--

To illustrate why onChange is currently called - consider this example:

import { createEffect, createSignal } from "solid-js";
import { Select } from "@thisbeyond/solid-select";

export const ResetExample = () => {
  const [initialValue, setInitialValue] = createSignal("apple", {
    equals: false,
  });

  createEffect(() => console.log("initialValue", initialValue()));

  const [displayValue, setDisplayValue] = createSignal(undefined);

  // Change to false to see current behaviour and why it is needed for those
  // listening to onChange to receive the correct current value.
  let initialising = true;

  return (
    <>
      <div class="flex-1 flex flex-col gap-3">
        <div class="flex gap-2">
          <Select
            initialValue={initialValue()}
            options={["apple", "banana", "pear", "pineapple", "kiwi"]}
            onChange={(value) => {
              if (initialising) {
                initialising = false;
                return;
              }
              console.log("on change!", value);
              setDisplayValue(value);
            }}
          />
          <button class="primary-button" onClick={() => setInitialValue(null)}>
            Reset
          </button>
        </div>
        <div>Value is: {displayValue()}</div>
      </div>
    </>
  );
};

By skipping the first onChange (as you want to do) the value displayed is stale on initialisation.

We could have initialValue not be reactive. And then require a different value prop if you want it to be reactive, but I feel that is a messier interface and would confuse more.

To be clear, I'm open to figuring something out here - just trying to share the why behind the current design 😄