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

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

Open
elliotwaite opened this issue Jun 17, 2023 · 3 comments
Open

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

elliotwaite opened this issue Jun 17, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@elliotwaite
Copy link

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?

@elliotwaite elliotwaite added the enhancement New feature or request label Jun 17, 2023
@bigmistqke
Copy link

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

@joshwilsonvu
Copy link
Collaborator

This should now be doable with #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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants