solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
216 stars 26 forks source link

Question: What's a tracked scope? #8

Closed imedadel closed 2 years ago

imedadel commented 2 years ago

Hello Josh! I'm enjoying this ESLint plugin, however, I'm having trouble understanding the meaning of a tracked scope and the examples aren't quite answering my question. In my case, I wanted to pass a prop to a custom hook/function but I'm getting either The reactive variable 'localRef' should be used within a tracked scope or This function should be passed to a tracked scope because it contains reactivity.

Example:

const Component = (props) => {
  const [local, rest] = splitProps(props, ["ref"]);
  const localRef = () => local.ref;
  const composedRef1 = useComposedRefs(localRef); // error: The reactive variable 'localRef' should be used within a tracked scope
  const composedRef2 = useComposedRefs(() => local.ref); // error: This function should be passed to a tracked scope because it contains reactivity.
    // ...
}

What am I missing? :weary:

And, sorry if this is not the right place to ask!

joshwilsonvu commented 2 years ago

Hi, thanks for the question, and I don't mind you asking here at all. Tracked scopes are mentioned a few times in the Solid docs; they're basically any function or expression where it's okay to have reactive variables because Solid is watching for changes. The idiomatic example is createEffect and JSX, but there's several Solid primitives that do this.

function Component() {
  const [signal, setSignal] = createSignal(1);
  setTimeout(() => {
    setSignal(2);
  }, 1000);
  createEffect(() => console.log(signal())); // tracked scope, logs 1, then 2 when signal changes

  return <div>{signal()}</div>; // tracked scope, displays 1, then 2 when signal changes
}

The reactivity rule tries to make sure that everywhere you use a reactive variable (signal, props, memos, etc., function containing any of these), Solid is able to watch for changes and do things like re-run effects and update HTML. Here's an example where this rule would warn because the console.log(signal()) is not in a tracked scope.

function Component() {
  const [signal, setSignal] = createSignal(1);
  setTimeout(() => {
    setSignal(2);
  });
  console.log(signal()); // only logs 1, never logs 2
}

I would have liked to link to an answer to you question in the error message, but couldn't find a good explanation.


Now, the reactivity rule is doing its best, but there are a lot of edge cases to uncover, and I think custom hooks is one of them. I could write a hook that works as a tracking scope (because it uses Solid primitives that create a tracking scope), but the rule would incorrectly warn.

const [signal] = createSignal(1);
function createEffectCopy(...args) {
  return createEffect(...args);
}
createEffectCopy(() => console.log(signal())); // warns, even though this is fine

There's not a perfectly accurate solution here: the reactivity rule can't know whether a custom hook creates a tracking scope or not. But I don't want to annoy the programmer, so the rule shouldn't automatically warn about using signals/props in a custom hook. You've discovered a bug!

Thanks for reading, hope that's helpful, and in a future release the rule won't warn when your hook is named use* or create*. I'll leave this issue open until that's fixed.

imedadel commented 2 years ago

Thank you for the extremely detailed answer, this definitely helped a lot!

joshwilsonvu commented 2 years ago

No problem! For now, just disable the rule on each line with /* eslint-disable-line solid/reactivity */. If you add "reportUnusedDisableDirectives": true to your ESLint config, it will tell you when the disable comment isn't needed anymore.

millette commented 2 years ago

Here's another example I'm having trouble with

https://github.com/millette/sobolid/blob/e827675473a2ef99bc8e1f21c762a1253e14b4d4/src/pages/credits.tsx#L77-L93

Specifically line 83. I'm also seeing the same warning with other <For> components.

Maybe it's late and I don't quite understand the rule, or maybe it's another edge case. I'd like to find out :-)

joshwilsonvu commented 2 years ago

Hi @millette, I double checked and I believe this case is actually reporting a bug. The offending code:

onClick={setSelected.bind(
  null,
  i() /* eslint-disable-line solid/reactivity */
)}

From the docs:

Events are never rebound and the bindings are not reactive, as it is expensive to attach and detach listeners.

Unlike React, where you render multiple times and .bind() creates a new function each render, your Solid component only runs once and the function you pass to onClick has to work by capturing i and calling it when needed.

In your code, setSelected is bound once to whatever the initial value of i is. In the example below, as the value in i changes over time, the event handler function gets the current value each time it runs.

onClick={() => setSelected(i())}

With Solid, there is no performance penalty to this code. The reactivity rule expects there to be a function here and lets you use signals and props within the function without warning.

Let me know if there's anything else I can help you with!

millette commented 2 years ago

@joshwilsonvu Thanks for looking into it. I checked the docs and also tried

<button onClick={[setSelected, i()]}>

That's still triggering the same warning.

When I use your suggestion, everything works and no warning. I believe the array version should also work without a warning.

millette commented 2 years ago

One last example, this time with createResource: https://github.com/millette/sobolid/blob/e65364e3b31c726986b221709119cdd107732da5/src/components/avatar-item.tsx#L15

joshwilsonvu commented 2 years ago

I checked the array syntax and it doesn't seem to work in practice. In my example it always logs the initial value and never updates, which fits with "the bindings are not reactive" from the docs. I think the warning is correct: you have to use a function if you're using signals or props. The array syntax seems to be for non-reactive arguments, like constant strings/numbers/etc.

With your createResource example, you're right that the first argument can be reactive, but it has to be a signal variable that's not called (createResource(signal, ...)). Solid will call it to determine when changes occur. In your case you need a derived signal, a function returning the prop: createResource(() => props.partsFileName, parseIt). See the docs.

joshwilsonvu commented 2 years ago

@ImedAdel The issue with custom hooks should be resolved in v0.4.4. Thanks again!