solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
216 stars 26 forks source link

Prefer Index to For when updating the collection within the loop #22

Closed Worble closed 1 year ago

Worble commented 2 years ago

Describe the need Updating the collection used by the <For> within the <For> causes any focused elements to lose focus, using <Index> does not have this issue. This may be a misinterpretation on my part, but reading the docs I couldn't see a way of keying <For>, which is the "React-y" way of achieving this.

Suggested Solution Is it possible to raise a lint warning when the signal setter is called within the <For> loop, advising users to use <Index> instead? As a minimal example:

const IndexTest: Component = () => {
    const [names, setNames] = createSignal<string[]>([""]);
    return (
        <>
            <div>
                <button onClick={() => setNames([...names(), ""])}>Add Name</button>
            </div>
            <div>
                <h2>For</h2>
                <For each={names()}>
                    {(name, index) => (
                        <input
                            value={name}
                            onInput={(e) => {
                                const clone = [...names()];
                                clone[index()] = e.currentTarget.value;
                                setNames(clone); // <-- setNames called within <For> using names signal, raise lint error here?
                            }}
                        />
                    )}
                </For>
            </div>
            <div>
                <h2>Index</h2>
                <Index each={names()}>
                    {(name, index) => (
                        <input
                            value={name()}
                            onInput={(e) => {
                                const clone = [...names()];
                                clone[index] = e.currentTarget.value;
                                setNames(clone); // <-- No lint raised
                            }}
                        />
                    )}
                </Index>
            </div>
        </>
    );
};
joshwilsonvu commented 2 years ago

It's not super clear, but the docs describe the <For /> component as a "referentially keyed loop." That means when your array changes, any values or objects that were previously included will be automatically "keyed" for you, so no need for an explicit key prop like in React. For example:

import { render, For, createSignal } from 'solid-js/web';

let [array, setArray] = createSignal([{ name: 'Alice' }, { name: 'Bob' }]);
render(
  <ul>
    <For each={array}>{(item, i) => (
      <li>{item.name} #{i()}</li>
    )}</For>
  </ul>
);
setTimeout(() => {
  // swaps "Alice" and "Bob", appends "Charlie"
  setArray([array[1], array[0], { name: 'Charlie' }]);
}, 1000);

setTimeout(() => {
  // recreates the entire list (expensive), since none of these objects are the same references as before
  setArray([{ name: 'Alice' }, { name: 'Bob' }, { name: 'Charlie' }]);
}, 2000);

The first setArray call triggers an update (a "re-render" in React terms, but without reevaluating any components), and since the <For /> has seen those specific { name: 'Alice' } and { name: 'Bob' } objects before, it knows to swap the lis instead of destroying and recreating them. It hasn't seen { name: 'Charlie' } before so it creates a new li for it. The second setArray call gets different references, so new DOM elements are created.

All that to say, if an element has focus and the array changes order, the updated DOM should use the same element, preserving focus.

Updating the collection used by the within the causes any focused elements to lose focus

Does your code look more like the second example? That would explain it.

Worble commented 2 years ago

Hi, sorry I'm still having issues wrapping my head around this, mostly regarding how to update an existing object in the array whilst keeping focus. If it's a referentially keyed, I'm assuming that means it works on the object reference? So does that mean we want to mutate the original object when making updates? I tried spreading the original array so the original setSignal triggers, while then modifying object in the array I wanted to update, however this does not cause that element in the For to rerender (but does outside the For).

A quick example below:

const ForTest: Component = () => {
  const [objectArray, setObjectArray] = createSignal<{ name: string }[]>([]);

  return (
    <>
      <button
        onClick={() =>
          setObjectArray((current) => {
            return [...current, { name: "" }];
          })
        }
      >
        Add
      </button>
      <br />

      <h3>Always re-renders</h3>
      {JSON.stringify(objectArray())}

      <h3>Mutation - Does not re-render</h3>
      <For each={objectArray()}>
        {(obj, idx) => (
          <>
            <input
              value={obj.name}
              onInput={(e) =>
                setObjectArray((current) => {
                  const clone = [...current];
                  clone[idx()]!.name = e.currentTarget.value;
                  return clone;
                })
              }
            />
            <div>{obj.name}</div>
            <br />
          </>
        )}
      </For>

      <h3>New object - Re-renders but loses focus</h3>
      <For each={objectArray()}>
        {(obj, idx) => (
          <>
            <input
              value={obj.name}
              onInput={(e) =>
                setObjectArray((current) => {
                  const clone = [...current];
                  clone[idx()] = { name: e.currentTarget.value };
                  return clone;
                })
              }
            />
            <div>{obj.name}</div>
            <br />
          </>
        )}
      </For>
    </>
  );
};
joshwilsonvu commented 2 years ago

You're right that it's keyed on the object reference, but I don't think it's advisable to mutate the object. Since using <input> in JSX is roughly equivalent to document.createElement('input'), rerunning the For function again for a new object is bound to lose focus.

Would you mind trying a different approach--putting the markup within the For into a new component and passing obj.* as props? I think that way you'll be updating the props of the component and it will be able to react to the chance without recreating the input. Let me know if that doesn't work, this is definitely something that might merit a new lint rule.

joshwilsonvu commented 1 year ago

Going to close since it's been a while—feel free to comment again if you think there's something specific the linter can help with here.