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

More complex examples or good/bad practices documented #8

Closed
gtktsc opened this issue Oct 30, 2019 · 10 comments
Closed

More complex examples or good/bad practices documented #8

gtktsc opened this issue Oct 30, 2019 · 10 comments

Comments

@gtktsc
Copy link

gtktsc commented Oct 30, 2019

Hey! Awesome work! I hope it's gonna be a part of React 🤞.

To be precise: I'm not raising issue, rather feature request - could you please extend documentation or add more complex examples? Especially part about good/bad practices. For example how to access function/objects stored in context because those must have same reference in order to skip update.
Example - component is getting function from context:

const addListener = useContextSelector(
    ListenerContext,
    listeners => listeners.addListener
)

Provider looks like:

const [listeners, setListeners] = useState([])
const [state, setState] = useState({}) // some other state
const addListener = (listener) => {
    // some complex logic 
    setListeners(newListeners)
}

const state = {
    state,
    addListener,
}

return (
    <Provider value={state}>
        {children}
    </Provider>
)

Therefore as compare condition is:

(ref.current.v === nextValue || Object.is(ref.current.s, ref.current.f(nextValue)))

Update will be triggered. However it might be fixed by useRef:

const { current: addListener } = useRef((listener) => {
    // some complex logic 
    setListener(newListeners)
})

I think there are more examples of how to use/how to not use this hook and it might be helpful to document it.

Cheers!

@dai-shi
Copy link
Owner

dai-shi commented Oct 31, 2019

Hey, thanks for your feedback.

Yeah, more examples in docs would be nice. I was actually hoping the RFC would be discussed actively and I wouldn't need to keep this repo if it were to be a part of React some time soon. So, I didn't prioritize documentation.

My current focus is documenting more for React Tracked. (Actually, this repo was born as a simpler version without useTrackedState.) So, if someone would volunteer for this repo, it would be nice.

I'm happy to answer questions. In your example, I think you need to memoize objects.

const [listeners, setListeners] = useState([])
const [state, setState] = useState({}) // some other state
const addListener = useCallback((listener) => {
    // some complex logic 
    setListeners(newListeners)
}, [...])

const value = useMemo(() => ({
    state,
    addListener,
}), [state, addListener])

return (
    <Provider value={value}>
        {children}
    </Provider>
)

Finally, @JoviDeCroock have incorporated use-context-selector in his hooked-form, so that might help as a practical example.
...and I noticed that he doesn't no longer have a direct dependency. Maybe look at the old PR?

@JoviDeCroock
Copy link
Contributor

JoviDeCroock commented Oct 31, 2019

I have converted to my own event emitter for the React warning and performance when using 1000+ useContextSelectors. This also gave some size advantages for me, I’ll answer with how I used it here tomorrow.

@dai-shi
Copy link
Owner

dai-shi commented Oct 31, 2019

At a glance, it's so tuned and tightly-coupled that it's not like a good example to explain a use case of use-context-selector. 😅

(I wonder if you really need a context value in hooked-form. I mean useSubscription might just work.)

@JoviDeCroock
Copy link
Contributor

use-subscription would impose needless bloat and an alternative thinking pattern to the Field/Form approach people are used to.

@dai-shi
Copy link
Owner

dai-shi commented Oct 31, 2019

use-subscription would impose needless bloat

I assumed that everything can be hidden from the library users, but I'm not suggesting it anyway.

@JoviDeCroock
Copy link
Contributor

What aspects are you interested in in specific @dai-shi I use an event emitter comparable to https://github.com/developit/mitt and the listeners are all registered in a useEffect.

Whenever a value/error/... changes the event is emitted to that one component, so only one component (or multiple if there are more listeners) will rerender. This seems to be concurrent friendly since all the changes to these watched values are invoked in useCallback and useEffect.

@dai-shi
Copy link
Owner

dai-shi commented Oct 31, 2019

a) how hooked-form used to use use-context-selector 👉 a good practice
I hope I (or someone) can write pseudo code to show a use case.

b) how hooked-form works now without it 👉 sort of a bad practice
Seems like a simple event emitter works well. How it would get a benefit from the non-propagating context.

@JoviDeCroock
Copy link
Contributor

JoviDeCroock commented Oct 31, 2019

A) it used to always use three separate useContextSelectors to get value, errors and touched with a recursive function as the selector. This meant that it only did string/null/undefined comparison resulting in an ideal scenario for useContextSelector. The functions were useCallback with an empty array so it was safe to derive these from a normal React.useContext. ensuring we are only doing simple comparisons avoids mistakes happening from the user-space.

So useField was basically, dumbed down example without errors and stuff.

const value = useCtxSelector(ctx, ({ values }) => get(values, fieldId));
const { setValue } = React.useContext(ctx);

return [value, setValue];

B) I can't see how this is a "bad" practice since this boosts performance and is concurrent friendly, one of the most popular form libraries final-form relies on a similar pattern only that it doesn't put the values on the context but an inhouse subscription client instead.

useField

on(fieldId, rerender);
const { setValue, values } = React.useContext(ctx);

return [get(values, fieldId), setValue];

@dai-shi
Copy link
Owner

dai-shi commented Oct 31, 2019

a "bad" practice

Oops, I meant a case that doesn't fit with useContextSelector as is. no?

@dai-shi
Copy link
Owner

dai-shi commented Jan 17, 2020

Closing this. Please open a new issue for further discussion.

@dai-shi dai-shi closed this as completed Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants