solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
209 stars 24 forks source link

False Positives for solid/reactivity #74

Closed PodaruDragos closed 1 year ago

PodaruDragos commented 1 year ago

Hello,

From the docs here reactivity-docs it seems there is a snippet in the valid examples :

function Component(props) {
  const [count, setCount] = useSignal(props.initialCount);
  return <div>{count()}</div>;
}

that shows a valid use of passing props value inside a signal.

However doing the same thing I get (property) ChildProps.selectedOption?: string | undefined The reactive variable 'props.selectedOption' should be used within JSX, a tracked scope (like createEffect), or inside an event handler function.eslint[solid/reactivity](https://github.com/solidjs-community/eslint-plugin-solid/blob/main/docs/reactivity.md)

I have made a quick example of valid code here: playground

Now using the plugin in vscode I get image

I simply can't understand what the issue is here, since the code is valid and as per docs it should also be valid. To note: Passing an Accessor as prop results in the same behaviour.

Using latest solid-js, latest eslint-plugin-solid.

joshwilsonvu commented 1 year ago

Hi, sorry for the confusion--you've stumbled on an escape hatch. See my answer here.

In short, since it's common to want to initialize a signal with a prop, but it's still a case where you're going to miss any reactive updates to that prop, the rule warns unless the prop name starts with initial or default. So the behavior you're seeing is intentional.

If you still want to initialize a signal with a prop but don't want to change its name, just disable the warning for that line.

joshwilsonvu commented 1 year ago

A bit of unsolicited advice--you might have an easier time storing a selectedId or selectedIndex instead of the actual selected item, especially if your list items become editable. That way there's no duplication between your list state and your selection state.

This idea is described more in these React docs; if you're unfamiliar with React don't worry about it but the same concepts apply to signals.

PodaruDragos commented 1 year ago

Hello, thanks for clearing this out, however

In short, since it's common to want to initialize a signal with a prop, but it's still a case where you're going to miss any reactive updates to that prop

I updated the example here playground using a derived signal. In the playground it seems to not error.

Inside of vscode I get

const option: () => string
The reactive variable 'option' should be used within JSX, a tracked scope (like createEffect), or inside an event handler function.eslint[solid/reactivity](https://github.com/solidjs-community/eslint-plugin-solid/blob/main/docs/reactivity.md)

A bit of unsolicited advice--you might have an easier time storing a selectedId or selectedIndex instead of the actual selected item, especially if your list items become editable. That way there's no duplication between your list state and your selection state.

Thanks for pointing this out aswell.

joshwilsonvu commented 1 year ago

Yes, thanks--the playground is using a slightly out of date version of the plugin from before a bug was fixed with deriving a signal using props. Your VSCode error is correct in warning you that changes in option() will be ignored.

PodaruDragos commented 1 year ago

Your VSCode error is correct in warning you that changes in option() will be ignored.

Yes, but should that be an error ? a derived signal is a signal, so the error there does not make sense. Or am I missing something ?

joshwilsonvu commented 1 year ago

Yes, I think so. Generally, this rule warns when it thinks you might expect reactivity to be tracked (something to happen when a prop/signal/store updates), but it can tell that nothing will happen. In all of the following lines, nothing happens when props.x changes since components run once, so the rule should warn, unless you've shown your intent to ignore the change by using the escape hatch with naming.

function Component(props) {
  createSignal(props.x);
  console.log(props.x);
  const option = () => props.x; createSignal(option());
}