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

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

Open
jfrere opened this issue Dec 20, 2022 · 4 comments
Assignees
Labels
bug Something isn't working reactivity v2 To be addressed in the rewrite of the solid/reactivity rule

Comments

@jfrere
Copy link
Contributor

jfrere commented Dec 20, 2022

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):

  • OS: Mac OS 13.1
  • Node version (node --version): 19.2.0
  • eslint-plugin-solid version (npm list eslint-plugin-solid/yarn why eslint-plugin-solid): 0.9.1
  • eslint version (npm list eslint/yarn why eslint): 8.29.0
@jfrere jfrere added the bug Something isn't working label Dec 20, 2022
@jfrere
Copy link
Contributor Author

jfrere commented Dec 20, 2022

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
Copy link
Collaborator

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
Copy link

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).

@joshwilsonvu joshwilsonvu changed the title Using createMemo inside mapArray triggers the solid/reactivity lint Reactivity rule does not allow directly returning a reactive expression instead of assigning to a variable Jan 28, 2023
@joshwilsonvu joshwilsonvu added the reactivity v2 To be addressed in the rewrite of the solid/reactivity rule label Jan 28, 2023
@aW4KeNiNG
Copy link

Same issue using the next code:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reactivity v2 To be addressed in the rewrite of the solid/reactivity rule
Projects
None yet
Development

No branches or pull requests

4 participants