solidjs-community / eslint-plugin-solid

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

Fixing unexpected "solid/reactivity" warnings #21

Closed pmwmedia closed 2 years ago

pmwmedia commented 2 years ago

When using stores and context providers, I get some "solid/reactivity" warnings, which I don't know to fix nor I understand what is actual the issue in these cases.

I have created a simple toggle component for demonstrating the warnings:

import {createContext, useContext} from "solid-js"
import {createStore} from "solid-js/store"

type ToggleContextType = [{ value: boolean }, { toggle: () => void }]
const ToggleContext = createContext<ToggleContextType>()

function ToggleDisplay() {
    const [state] = useContext(ToggleContext)
    return <span>{state.value ? "+" : "-"}</span>
}

function ToggleButton() {
    const [, {toggle}] = useContext(ToggleContext)
    return <button onclick={toggle}>Toggle</button>
}

export function Toggle(props: {value?: boolean}) {
    const [state, setState] = createStore({ value: props.value ?? false }) /* warning #1 */
    const contextValue: ToggleContextType = [
        state,
        { toggle: () => setState({value: !state.value}) } /* warning #2 */
    ]

    return (
        <ToggleContext.Provider value={contextValue}>
            <ToggleDisplay />
            <ToggleButton />
        </ToggleContext.Provider>
    )
}

Warning # 1

18:52  warning
The reactive variable 'props' should be used within JSX, a tracked scope (like createEffect), or an event handler.
Details: https://github.com/joshwilsonvu/eslint-plugin-solid/blob/main/docs/reactivity.md  solid/reactivity

Why does the plugin says that initializing a store by a property is bad? What would be a better alternative?

Warning # 2

21:22  warning
This function should be passed to a tracked scope (like createEffect) or an event handler because it contains reactivity.
Details: https://github.com/joshwilsonvu/eslint-plugin-solid/blob/main/docs/reactivity.md  solid/reactivity

Actually, accessing the value for toggling inside of the context function seems to work. However, is there a better way to perform the toggling?

joshwilsonvu commented 2 years ago

Thanks for using the plugin, happy to answer your questions.

Warning # 1

The general idea of this rule is that things that you expect to change over time (like props.value) should only be used in places where Solid can react to those changes (like createEffect or JSX), or in event handlers. It's warning here because the createStore initializer can't react to changes in the prop.

Sometimes you want to do this anyway, when you only care about the initial value of the prop. The rule does have an escape hatch for that: if the prop name starts with initial or default i.e. initialValue, it won't warn because that shows that you understand that in copying props to state, you won't use changes to the prop. This is a fairly common convention for that purpose. If that's not what you intended, I would suggest not using a store.

Warning # 2

You're getting a warning here because there's no way for a linter to determine how your toggle function will be used. Because it contains a read to state.value, if someone were to call toggle inside a createEffect or something, the effect would re-run each time state.value changes, which is likely not what you intended. For updating stores, Solid does have an updater function syntax that will fix the warning:

{ toggle: () => setState("value", v => !v) }

Here you're not actually reading from the store so the lint rule has nothing to warn about. (Using untrack might also work but the updater function is more idiomatic.)

Let me know if you have other questions!

pmwmedia commented 2 years ago

Thank you very much for your quick response and well explained answer. I really like the self-explaining initialValue naming convention and I'm also using the updater function now. So, both warnings are fixed and I could even improve the readability of my code.

Many thanks for your awesome ESLint plugin!