Issues with hooks in useStoryblok

[johannes-lindgren]

There are a few issues with the useStoryblok():

1. useEffect() is called conditionally on line 40, caused by early return statement on line 35. See the rules of hooks
2. useSbBridge() causes side-effects, yet it is not called within `useEffect(). (Despite its name, useSbBridge is not a react hook)
3. useStoryblokBridge() cause side-effects that are not cleaned up. How do we prevent memory leaks? What if the registered events try to mutate the state of a component that has unmounted?
4. On line 47, we might be mutating the component's state after it has unmounted. This will happen if the component is unmounted after sending the fetch request, and before the setState() is called. See https://www.codingdeft.com/posts/react-prevent-state-update-unmounted-component/
5. I am not completely sure about this one: In useStoryblokBridge(), we are subscribing to events from the bridge. If two different components are calling useStoryblokBridge(), then I assume that line 37 will be called twice; one will pass the condition and result in a mutated state, and the other will cause the window to reload. That's a side-effect that is not easy to foresee. Since we are not cleaning up the event listeners (I think), anytime we unmount the story component and replace it with another story component, the window will reload.

[bertrand-riviere]

i also think it is missleading to use the "useStoryblokBridge" name inside storyblok-react lib because as far i can see it is simply a re-export from @storyblok/js which has nothing to do with React:

storyblok-react/lib/index.ts

Line 21 in 56a8adf

} from "@storyblok/js";

if a user directly uses it as if it was a React hook (at the root of the render func) the event will gets added again and again, render after render.

i think you should implement a proper React hook in this lib (wrapping the call to useStoryblokBridge in a useEffect with a the callback, etc.. as dependencies + do the needed cleaning up as johannes-lindgren mentionned) or at least you should make it very clear in the doc that useStoryblokBridge is NOT a hook as its name suggests.

[CAYdenberg]

I have a similar issue with useStoryblokState which is causing my previews to crash.

This useEffect (

storyblok-react/lib/common/client.ts

Line 22 in 60cd8de

useEffect(() => {

) is called conditionally.

I would really suggest using React's eslint plugin to enforce the Rules of Hooks.

[yannic-wtfoxtrot]

I think the first issue is the most annoying as it causes the following error with each HMR update: Error: Rendered more hooks than during the previous render.

I just noticed it in my Next js application and found it pretty hard to narrow down. From my understanding this could be easily resolved by moving the conditional to the useEffect itself. I'm going to create a PR and would greatly appreciate feedback here :)

[yannic-wtfoxtrot]

I just created a PR for this #870. I'd appreciate if someone could have a look at it :)

[whlufisc]

Hello 👋!
Unfortunately PR #870 breaks the useStoryblokState hook.

Right now the useEffect hook is executed everytime story changes.
Now the following happens if the callback (newStory) => setStory(newStory) is executed:

1. Triggers a rerender -> story returned by useState
2. Effect is executed again
3. setStory(initialStory); resets the state variable back to its initial value
4. Another rerender is triggered with the old initialStory being returned by useState

When trying to edit something this can be observed in the live editor by content jumping around when something is changed, because the change is only displayed for one render.

This can be solved by pulling const id = (story as any).internalId || story.id back out of the useEffect-hook and using it as a dependency of the effect.

const useStoryblokState: TUseStoryblokState = (initialStory = null, bridgeOptions = {}) => {
    const [story, setStory] = useState(initialStory)

    const isBridgeEnable = typeof window !== 'undefined' && typeof window.storyblokRegisterEvent !== 'undefined'
    const id = (story as any).internalId || story.id

    useEffect(() => {
        if (!isBridgeEnable || !initialStory) return
        setStory(initialStory)
        registerStoryblokBridge(id, (newStory) => setStory(newStory), bridgeOptions)
    }, [initialStory, isBridgeEnable, id])

    if (!isBridgeEnable || !initialStory) {
        return initialStory
    }

    return story
}

If you would want to adhere to the rules of hooks completely, including exhaustive-deps, you would have to include bridgeOptions as a dependency of the effect. But I would not recommend this because this object will not be stable when this is overriden by the user.

Overall I don't think it is necessary here to be that strict with the dependencies in general. The function registerStoryblokBridge comes from another repo, so you will always have to make sure what happens if this function is called multiple times because of rerenders or you make sure that it is actually only called once.

[yannic-wtfoxtrot]

@whlufisc sounds good to me. Can you prepare a pull request?

[whlufisc]

Someone else was faster #883

[franzi-wtfoxtrot]

Hello, is there anyone (maybe @fgiuliani ) who could do a review of the PR #883 ? The non-functioning preview is unsettling our editors and annoying us, as we cannot provide any other solution to the problem at the moment. Thanks in advance!

[arorachakit]

Hey @franzi-wtfoxtrot @whlufisc - The PR was merged a couple of weeks back :)

Maybe we can close this issue now @johannes-lindgren ?