solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
209 stars 24 forks source link

Reactivity rule does not allow directly returning a reactive expression instead of assigning to a variable #52

Open jfrere opened 1 year ago

jfrere commented 1 year ago

Describe the bug

Returning the result of createMemo inside of mapArray triggers the solid/reactivity lint, but in practice this isn't an issue.

To Reproduce

  const rowHeights = mapArray(
    () => props.items,
    (item) =>
      createMemo(() =>  // <-- lint happens on this line
        typeof props.rowHeight === "number"
          ? props.rowHeight
          : props.rowHeight(item)
      )
  );

Lint text:

"For proper analysis, a variable should be used to capture the result of this function call."

Expected behavior

I expect there to be no warning present.

(Or, if there should be a warning present, then maybe this is a documentation issue? I can avoid this warning by writing const m = createMemo(...); return m, which seems to me to be the same thing, but I might be missing something!)

Environment (please complete the following information):

jfrere commented 1 year ago

Possibly a similar issue, I'm also getting this spurious (I think?) lint:

export function createCustomStore(): Accessor<CustomItem[]> {
  const [store, updateStore] = createStore({
    /* ... */
  });

  return mapArray(
    () => store.path.to.field, // lint happens on this line
    (item) => ({
      /* ... */
    })
  );
}

In this case, the lint is "this function should be passed to a tracked scope (like createEffect) or an event handler because it contains reactivity", which I don't think applies in this case, because the first argument of mapArray is a tracked scope.

I can create a new issue for this if that's more helpful.

joshwilsonvu commented 1 year ago

You're right, the "For proper analysis, a variable should be used to capture the result of this function call." warning is not necessarily a real problem, but a limitation of the lint rule. It currently operates on variables only and can't analyze arbitrary expressions. It warns because you might be missing analysis that would give you a real warning, but since you've tried rewriting it to use a variable you're in the clear.

I'll keep this issue open until that limitation is relaxed or removed, because I'd like it to be a little smarter. Let's move your second thing to a new issue, I didn't realize mapArray was a tracked scope.

russelldavis commented 1 year ago

I just ran into this as well. This may already be your plan, but a simple fix that would cover the most common case would be to not show the warning when the reactive value is being returned from a function (either explicitly with return, or implicitly as part of a single expression arrow function).

aW4KeNiNG commented 1 year ago

Same issue using the next code:

<button
    type="button"
    classList={mergeProps(local.classList ?? {}, {
        btn: true,
    })}
    {...others}
/>