solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
220 stars 27 forks source link

Add ability to mark a function as a tracked scope #98

Open elliotwaite opened 1 year ago

elliotwaite commented 1 year ago

Describe the need

In this example...

import { useLocation } from "@solidjs/router"
import { createComputed, VoidProps } from "solid-js"

function onLocationPathnameChange(fn: () => void) {
  const location = useLocation()

  createComputed((isInitialRun) => {
    location.pathname
    if (!isInitialRun) {
      fn()
    }
  }, true)
}

function MyComponent(
  props: VoidProps<{ handleLocationPathnameChange: () => void }>,
) {
  // *** This line raises the error: ***
  onLocationPathnameChange(() => props.handleLocationPathnameChange())

  return <div>Hello</div>
}

...the onLocationPathnameChange(() => props.handleLocationPathnameChange()) line raises this error:

ESLint: This function should be passed to a tracked scope (like createEffect) or an event handler because it contains reactivity, or else changes will be ignored.(solid/reactivity)

I assume this is because the linter can't tell that onLocationPathnameChange will run the passed-in fn function inside of a createComputed. I assume this is the case because if I inline the onLocationPathnameChange function, it works without an error:

function MyComponent(
  props: VoidProps<{ handleLocationPathnameChange: () => void }>,
) {
  const location = useLocation()

  createComputed((isInitialRun) => {
    location.pathname
    if (!isInitialRun) {
      props.handleLocationPathnameChange()
    }
  }, true)

  return <div>Hello</div>
}

It would be nice if there was a way to specify that a function would act as a tracked scope, or to specify that a closure argument of a function will be called within a tracked scope.

Suggested Solution

Perhaps there could be a way to specify that a function will act as a tracked scope similar to createComputed or createEffect. For example:

// eslint-plugin-solid tracked-scope-function
function onLocationPathnameChange(
  fn: () => void,
) {
  const location = useLocation()

  createComputed((isInitialRun) => {
    location.pathname
    if (!isInitialRun) {
      fn()
    }
  }, true)
}

Or be able to specify that certain closure arguments will be called in tracked scopes:

function onLocationPathnameChange(
  // eslint-plugin-solid tracked-scope-argument
  fn: () => void
) {
  const location = useLocation()

  createComputed((isInitialRun) => {
    location.pathname
    if (!isInitialRun) {
      fn()
    }
  }, true)
}

But this is just the first idea that came to mind. I'm not familiar with how eslint works so it's hard for me to offer any confident suggestions.

Possible Alternatives

Or maybe this information could be provided to the linter in some other way besides comments. I saw in the Implementation v2 notes that one of the goals was to add better support for extensibility. Would the planned additional extensibility be able to solve this issue?

bigmistqke commented 1 year ago

+1 this would greatly help library-authors in preventing false positives.

joshwilsonvu commented 9 months ago

This should now be doable with https://github.com/solidjs-community/eslint-plugin-solid/pull/113. It's a bit coarser than what you're suggesting, but hopefully should be helpful.

I'd definitely prefer to do something closer to your // eslint-plugin-solid comments! But ESLint rules can't easily look at function definitions located outside of the file it's currently linting. (The only hope of doing so is by integrating TypeScript's type-checking into lint rules, which can cause performance issues and requires some setup, but is worth considering.) So presently, we'd have to include one of these comments at each callsite, which isn't much more pleasant than just disabling the warning at each callsite.

Hopefully, after rearchitecting solid/reactivity a bit, it'll be easier to add smarter functionality.

elliotwaite commented 9 months ago

@joshwilsonvu, nice. That seems like a good option for now. Thanks for the update!

Also, similar to what @CreativeTechGuy mentioned in this comment, I wasn't aware of the create* and use* escape hatches. Those seem like good options too.