Warn about set signal in effects

[milomg]

Describe the need

Generally using setters inside an effect is suggestive of a problem with data structuring, and it prevents Solid's runtime from automatically doing analysis of reactive dependencies.

This is an example of potentially unsafe code

const [x, setX] = createSignal(y());

createEffect(() => {
  setX(y());
});

Safe code might be:

const x = createMemo(() => deriveXFromY(y()))
const setX = (newX) => setY(deriveYFromX(newX))

Suggested Solution

Ideally, a new lint would be added to the reactivity lint that looks for code and recommends restructuring the signals to avoid this pattern.

Possible Alternatives

Using the following helper function (this may be included in the Solid core in a future version)

function createWriteable<T>(getter: () => T): Signal<T> {
  const memoizedGetter = createMemo(getter);
  const wrapped = createMemo(() => {
    const [get, set] = createSignal(memoizedGetter());
    return { get, set };
  });
  return [
    () => wrapped().get(), 
    ((newval) => wrapped().set(newval)) as Setter<T>
  ];
}

Then the following code (which should fail the lint)

const [x, setX] = createSignal(y());

createEffect(() => {
  setX(y());
});

Can be rewritten as follows:

const [x, setX] = createWriteable(() => y())

Additional context

There is a similar issue with stores, but createWriteable doesn't work there (too high perf cost), so it is an open question if there are any easy fixes in that case.

• [ ] I would be willing to contribute a PR to implement this feature

[joshwilsonvu]

Fortunately, I think this will be simple to implement as its own rule outside of solid/reactivity so it can be maintained and enabled separately. This is limited to createSignal and createEffect only I presume?

Supplying an auto-fix could be more difficult, and I’m not totally convinced that a writable is much better of a solution, so I think just warning makes sense here.

[milomg]

Awesome! This rule applies to any combination of signals/stores, and effect/computed/renderEffect/memos. And yeah, maybe linking to a docs page/this issue for ideas on how to restructure your state to fix this lint would be good.

It may currently be impossible to fix the issue for all stores without a new core primitive, so ideally that part of the lint would be toggleable.

[snnsnn]

What about conditionals, they make using setters inside an effect a valid use case.