solidjs-community / eslint-plugin-solid

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

What is the purpose of noWrite in rules/reactivity.ts? #35

Closed aercolino closed 1 year ago

aercolino commented 1 year ago

What is the purpose of noWrite in rules/reactivity.ts?

I see that it's taken into account but not tested.

I use a pattern to pass signals to children like this:

function Parent(props) {
  let value, setValue;

  createRenderEffect(() => {
    [value, setValue] = createSignal(props.defaultValue); // <-- here
  });

  return <Child value={value}, setValue={setValue} />
}

And the noWrite rule gets triggered onto "value" there, but that's clearly not a problem because the reassignment is just the way to set value to the accessor and be able to use it outside of the createRenderEffect.

Should I setup things differently? (I'm new to Solidjs)

aercolino commented 1 year ago

I just found out a different setup, that works equally well:

function Parent(props) {
  const [value, setValue] = createSignal(null);

  createRenderEffect(() => {
    setValue(props.defaultValue);
  });

  return <Child value={value}, setValue={setValue} />
}

However, the question stands.

joshwilsonvu commented 1 year ago

I appreciate you taking the time to look through the code! The big reason the rule asks you to make signals, etc. const is so that it can more reliably analyze what's going on. (It's also more idiomatic Solid code, emphasizing that value is a function.)

If you have a variable that is sometimes a signal and sometimes not, you're inevitably going to run into false positives/negatives with warnings because that's impossible to track. We think that avoiding that is worth asking you to make certain variables const.

As an aside, your first example has problems that your second example fixes. If props.defaultValue changed, the effect would rerun and reassign value to a new signal, rather than updating the signal's value. You'd get problems with stale references Since components only run once, the child component will never even get the signal, and that's certainly not what you want. In both examples, you'll want to call value in the JSX, so that the prop is the value, not the signal function itself.

Ideally, you might want to write that pattern as the following.

function Parent(props) {
  const [value, setValue] = createSignal(props.defaultValue);

  return <Child value={value()} setValue={setValue} />
}

Thanks for the question!

goenning commented 1 year ago

hey @joshwilsonvu I have a similar code as suggested above, but I'm getting a rule warning on it. Is this expected?

Screen Shot 2022-10-31 at 05 18 56
joshwilsonvu commented 1 year ago

Yes, but you can & should ignore those warnings with // eslint-disable-next-line solid/reactivity as long as you understand that if those values change, createSignal won't react to it.

The rule is there to let you know about cases where you might expect Solid to react to a change in a prop/signal but it won't. Using props to initialize a signal is a fairly common case where this is okay, so there's an escape hatch where if your prop name starts with initial/default it won't warn, because you've shown that you intend to ignore any changes. For other cases like yours ignoring the warning accomplishes the same thing.