useEventListener does not check to see if event target is valid, which can break SSR #978

Activity

1. JoeDuncko

   added

   🪲 bugSomething isn't workingSomething isn't working

   on Oct 22, 2022
2. ArttuOll commented on Oct 29, 2022

   

   ArttuOll

   on Oct 29, 2022

   Last edited by ArttuOll

   Contributor

   More actions

   How should we go about fixing this?

   Could it be just a simple isBrowser check before adding the event listener or maybe even setting the target parameter to null if isBrowser is false?

   I attempted this, but found that I couldn't write a test which would reveal this bug, since in the SSR tests window can't be referenced at all. In fact, I launched a fresh NextJS project and learned that it won't even build if I give window to useEventListener directly. All of this makes sense in a SSR setting. The workaround suggested in the Discord by damntv works in the tests and NextJS: useEventListener(isBrowser ? window : null, ..., ...).

   This begs the question: how would we move the isBrowser check inside of the hook in order for the users to be able to just do this in SSR:

   useEventListener(window, ...);

   Is this even possible (disclosure: I have little experience with SSR)?
3. JoeDuncko commented on Nov 2, 2022

   

   JoeDuncko

   on Nov 2, 2022

   ContributorAuthor

   More actions

   I attempted this, but found that I couldn't write a test which would reveal this bug, since in the SSR tests window can't be referenced at all.

   Doesn't that actually test the bug exactly? Assuming we don't have window when running SSR tests, that should be a great test case as that's the specific error we are trying to avoid not get during SSR.

   My question is, do we have window provided during our not-SSR tests? If so, then I think that we could have this test in both test suites - and make sure the hook does not run but does not error during SSR, and does run client side.
4. ArttuOll commented on Nov 4, 2022

   

   ArttuOll

   on Nov 4, 2022

   Last edited by ArttuOll

   Contributor

   More actions

   My question is, do we have window provided during our not-SSR tests? If so, then I think that we could have this test in both test suites - and make sure the hook does not run but does not error during SSR, and does run client side.

   We provide null during tests. Providing window makes the tests fail, because window cannot be referenced server-side, which is why it cannot even be passed to useEventListener server-side. useEventListener(isBrowser ? window : null, ..., ...) works, because if isBrowser === false then window is not evaluated. If we moved the check inside the hook and passed window directly to the hook, window would be evaluated and error thrown.

   The hook itself uses the provided event target only inside of an useEffect which runs only client-side.

   Seems to me there is no bug here at all - or I am totally wrong and learn a lesson on SSR!
5. ArttuOll commented on Nov 4, 2022

   

   ArttuOll

   on Nov 4, 2022

   Contributor

   More actions

   This makes me wonder, should we then perhaps support giving globalThis as the target as suggested in #961 ? Then users could just useEventListener(globalThis, ...) and everything should work. Currently the type of the target parameter does not allow this.
6. ArttuOll commented on Nov 26, 2022

   

   ArttuOll

   on Nov 26, 2022

   Contributor

   More actions

   Any more thoughts on this? If not, I think this issue could be closed. Would love to see the issue list shrink a bit more!
7. xobotyi commented on Dec 18, 2022

   

   xobotyi

   on Dec 18, 2022

   Contributor

   More actions

   As figured - types has changed and there is no more issues with global this, but useEventListener still not guarded against improper during SSR.
   Im planning to address this issue by changing the way eventlistener added to our internal on and off methods that has target check. (on/off methods been introduced later that useEventListener)
8. xobotyi

   closed this as completedon Dec 20, 2022
9. xobotyi commented on May 2, 2023

   

   xobotyi

   on May 2, 2023

   Contributor

   More actions

   It has to be done by user, since user evaluates the window.
   isClient() ? window : null works because isClient uses typeof window under the hood and thus it wont break.

   Making target optional and placed as last call parameter will break arguments inferrence and is kinda weird.

   nextjs users shall use globalThis to avoid issues during SSR since we have nothing to do with evaluation order - no matter where you'll place target parameter - attempt to evaluate window during SSR will case a reference error.
10. ArttuOll commented on May 2, 2023

    

    ArttuOll

    on May 2, 2023

    Contributor

    More actions

    Just to note, this hook could also probably be reimplemented with React 18's useSyncExternalStore hook, which allows us to separately define the server-side behavior. But this goes back to the discussion and problems in #1023 .
11. xobotyi commented on May 2, 2023

    

    xobotyi

    on May 2, 2023

    Last edited by xobotyi

    Contributor

    More actions

    @ArttuOll yes, but that does not solve the issue with attempt to pass window object during SSR.
    we literally can do nothing since issue lays within user's code.
12. ArttuOll commented on May 2, 2023

    

    ArttuOll

    on May 2, 2023

    Contributor

    More actions

    @ArttuOll yes, but that does not solve the issue with attempt to pass window object during SSR. we literally can do nothing since issue lays within user's code.

    Yeah, you're totally right. That was just a quick thought.
13. xobotyi commented on May 3, 2023

    

    xobotyi

    on May 3, 2023

    Last edited by xobotyi

    Contributor

    More actions

    Alternatively we can change the logic to: if target is null, set target to window

    This is bad bad design with inobvious logic, we definely wont follow react-use's way.

    Even if there is no change being made in the function signature (which seems necessary to fix this)

    It is not necessary. useEventListener accepts EventTarget along with null as first argument. Hook designed to be similar with addEventListener (except acceptance of null target).

    can we at least add the information that we should set target as globalThis for ssr to work properly in both useEventListener docs and the migration guide?

    Not all SSR engines have globalThis support. I really don't understand why you blaming hook for reference error that is caused by consumer's (hence your's) code. It seemed to me that SSR users shall be cautios with window evaluation and understand the execution vlow and how to overcome the need of it's referencing.
    We physically unable to cover docs with all usecases that are possible within SSR (surprise - nextjs ist the only).
    Attempt to evaluate window on server will cause a reference error and it is beyond hook library competence, as most SSR engines has it's own way to reference the window object safely (most of them leaning toward globalThis nowadays but not all).

    The reason react-use assumed that by default developers want to add listeners to window - a big question to streamich.
    We've never targeted to make things compatible with react-use, the target is to provide more obvious and more predictable hooks withut silly defaults and assumptions hiddent behind hook's facade.
14. xobotyi commented on May 3, 2023

    

    xobotyi

    on May 3, 2023

    Last edited by xobotyi

    Contributor

    More actions

    The only thing we can do is to add notification that SSR users have to consult the docs of their SSR engine to safely reference the window object (it is complete surprise for me someone don't understand that though).
    @ArttuOll could you please address it within migration guide, maybe?
15. ArttuOll commented on May 5, 2023

    

    ArttuOll

    on May 5, 2023

    Contributor

    More actions

    The only thing we can do is to add notification that SSR users have to consult the docs of their SSR engine to safely reference the window object (it is complete surprise for me someone don't understand that though). @ArttuOll could you please address it within migration guide, maybe?

    I might have time for this next week.