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

Server side rendering #68

Closed
simmo opened this issue May 13, 2019 · 33 comments
Closed

Server side rendering #68

simmo opened this issue May 13, 2019 · 33 comments
Assignees
Labels
enhancement New feature or request released on @beta request for comment Discussion about changes to the library

Comments

@simmo
Copy link

simmo commented May 13, 2019

Describe the feature you'd like:

Support for server side rendering. I have rough implementation in the gist below... me than happy to drop this in a PR.

Suggested implementation:

https://gist.github.com/simmo/9b8d9fb13b5fba513a76a8413eeeb805

Describe alternatives you've considered:

I haven't seen any... yet.

Teachability, Documentation, Adoption, Migration Strategy:

I guess an additional export? Happy to take some guidance! 😄

@simmo simmo added the enhancement New feature or request label May 13, 2019
@mpeyper
Copy link
Member

mpeyper commented May 13, 2019

Looks cool.

I wonder how this would feel as an option to renderHook instead of a seperate function? I imagine it's only the initial render that would use the server renderer and subsequent rerender calls would use the existing renderer (with rehydration or something)? I'm quite unfamiliar with server-side rendering, so this might be nonsense.

My other thought is that we recently changed to use react-test-renderer so that we don't need a DOM for testing the hooks. Does this get undone by renderToString from react-dom or does the fact it's the server mean it's still not required?

@simmo
Copy link
Author

simmo commented May 14, 2019

Hey @mpeyper

I'm thinking out-loud here... I was wondering if we could flag react-dom as an optional dependency as, like you mention, SSR might not be the no.1 use case.

And, yes, you raise a valid point about hydration. For example you'd maybe expect a useInterval hook (example) to return an initial state and then hydrate on the client, triggering useEffect, executing the setInterval.

I'll have a play with react-test-renderer and see what I can come up with. A bit of a head-scratcher!!! 🤔

@mpeyper
Copy link
Member

mpeyper commented Jun 2, 2019

Hey @simmo,

just wondering how you're going with this and if there is anything I can help you with, or if this idea is dead and I can close this issue?

@mpeyper
Copy link
Member

mpeyper commented Jun 22, 2019

Closing this due to inactivity. Happy to reopen if anyone wants to start the discussion back up.

@mpeyper mpeyper closed this as completed Jun 22, 2019
@fabb
Copy link

fabb commented Jan 27, 2020

It would be really helpful to have this. I have a hook that produces different values on SSR and CSR based on useEffect and media queries. It would be great if there was a way to unit test this.

@mpeyper
Copy link
Member

mpeyper commented Jan 27, 2020

I'd love some help with this as SSR is completely foreign to me. I don't even understand what is failing/missing in the current implementation to begin looking at this.

I'll reopen this now as I'd also like to see us support more use cases.

@mpeyper mpeyper reopened this Jan 27, 2020
@mpeyper mpeyper added the help wanted Extra attention is needed label Jan 27, 2020
@fabb
Copy link

fabb commented Jan 27, 2020

On SSR, useEffect hooks are not run.

@mpeyper
Copy link
Member

mpeyper commented Jan 27, 2020

Of course, yes, that's a thing, isn't it... So rendering with react-dom/server skips the effects, but react-test-renderer does not.

I mentioned it before, but would it be important to you that the hook be able to rehydrate and start client rendering, or is just the initial server render the important part for your tests?

@mpeyper
Copy link
Member

mpeyper commented Jan 27, 2020

So I've briefly looked into it, and there does not appear to be a way to use react-test-renderer to hydrate a component from react-dom/server (please let me know if I'm wrong).

So I see three scenarios:

  1. Client render
  • Render with react-dom, rerender with react-dom
  1. Server render
  • Render with react-dom/server, rerender by hydrating with react-dom
  • Possibly a new API instead of rerender?
  1. Non-DOM render (e.g. react-native)
  • Render with react-test-renderer, rerender with react-test-renderer
  • Is there a better renderer to use for native rendering?

Are there any others I'm not thinking about?

I'm not sure how we can cleanly support all these different renderers at this stage, short of abstracting them away and having some way to configure which one to use.

This is starting to feel a bit like the enzyme adapter's and I'd really like to avoid that if we can.

Any suggestions are welcome.

@fabb
Copy link

fabb commented Jan 27, 2020

For my tests I don‘t really care for hydration, SSR-only and CSR-only suffices for my needs.

@mpeyper
Copy link
Member

mpeyper commented Jan 27, 2020

Thanks @fabb. If we implement anything I'd like it to be a complete solution, so I'll strive for that first, but if it's not possible, knowing what is important to users is helpful.

@simmo
Copy link
Author

simmo commented Jan 29, 2020

The only solution I was able to come up with was the one I suggested in my initial comment above. I’ve been using it for a while and it seems to be ok, however the need to declare a different env in the server.test.js is a bit annoying! I’m all out of ideas!

@fabb
Copy link

fabb commented Jan 29, 2020

The renderToString Method works fine for me, but I needed to use a custom wrapper because I use styled-components and need the ThemeProvider. To fix that I just copied the code from vanilla react-hooks-testing-library and replaced the render with renderToString, removed Suspense, and removed all the other no relevant stuff like rerender.

@mpeyper
Copy link
Member

mpeyper commented Jan 30, 2020

I've had a go at this and I've got something working for the scenarios described above, but i'm not sure about it so I'd love to get some feedback.

Basically, there would now be 3 ways to import renderHook to get slightly different rendering and act behaviour:

  1. import { renderHook, act } from '@testing-library/react-hooks/dom'
    • uses react-dom for initial render and rerender
    • uses act from react-dom/test-utils
    • requires a DOM in the test environment
    • supports Suspense
  2. import { renderHook, act } from '@testing-library/react-hooks/server'
    • uses react-dom/server for initial render and hydrates with react-domfor rerender
    • uses act from react-dom/test-utils
    • requires a DOM in the test environment
    • does not support Suspense (this is apparently a restriction for SSR?)
  3. import { renderHook, act } from '@testing-library/react-hooks/native'
    • uses react-test-renderer for initial render and rerender
    • uses act from react-test-renderer
    • does not require a DOM in the test environment
    • supports Suspense

The way I currently have it is that if you don't specify dom|server|native then it will use native as a default because it does not require a DOM and supports Suspense, but I think the vast majority of users will have a DOM and already have react-dom as a dependency, so I'm wondering the dom variant makes a more sensible default? This would make it a breaking change to the existing API, which might not be a bad thing given the structural changes happening in the internals of the package.

The other issue with what I currently have is that it has a peerDependency on both react-dom and react-test-renderer when most people will likely only ever need one. Ideally this would not be the case, and I'm going to look into optionalDependencies as an option to mitigate this, but the cost leaving it this way is either:

  1. a warning during installation of the user's project if the alternative renderer is not installed; or
  2. an extraneous dependency has to be installed to suppress the warning

I don't feel like the cost of either outcome here is worth not pursuing this, but I'm open to suggestions on other ways we could deal with the one or the other or both nature of the dependencies. The only other thought I've had is to actually release three seperate packages, e.g.

  1. @testing-library/react-hooks-dom
  2. @testing-library/react-hooks-server
  3. @testing-library/react-hooks-native

While this would be technically possible, it doesn't feel like the right approach to be taking, but again, I'd like to hear from anyone what their preferences would be (and why). One thing I like about what I've currently got is that you can install one package and test your hooks in a variety of ways, rather than installing 3 that do effectively the same thing.

That feels like a lot to take in, so I'll stop here for now. Please, I want as much feedback on this as I can get before I put too much effort into going down the wrong path. Happy to hear any thoughts on it.

@mpeyper mpeyper added the request for comment Discussion about changes to the library label Jan 30, 2020
@fabb
Copy link

fabb commented Jan 30, 2020

Using the same name renderHook for all 3 variants could be slightly confusing for auto-imports in the IDE. Apart from that it sounds good to me.

@mpeyper
Copy link
Member

mpeyper commented Jan 30, 2020

I don't think the naming of renderHook is up for debate here, unless anyone has a reasonable suggestion on what the alternative names might be.

If they were named differently, then there would be no reason to have the dom|server|native sub packages and just have differently named exports, e.g. import { renderHook, serverRenderHook, nativeRenderHook, act, nativeAct } from '@testing-library/react-hooks.

This looks kind of ok for ...renderHook, but the seperate act calls looks a bit weird. I wonder if there is a way for us to export our own act that picks the correct act to use based on the renderer? Or we could move it from a global export to rather be returned from renderHook and let the renderer decide which to return?

Another option is to only seperate native and have dom/server as the default:

  1. import { renderHook, serverRenderHook, act } from '@testing-library/react-hooks
  2. import { renderHook, act } from '@testing-library/react-hooks/native
    • no serverRenderHook for native as it doesn't make sense (does it?)
    • could be a seperate package `@testing-library/react-native-hooks
      • I'd rather keep the packages together as lots of the hooks people write for libraries are not specifically dom or native

I think I'm starting to head into the too many options zone and getting a bit of analysis paralysis on it. The biggest thing preventing me from pushing forward with this is the extraneous peerDependencies. Perhaps people don't care about that as much as I do, especially in their devDependencies and it's not a bit deal.

@mpeyper
Copy link
Member

mpeyper commented Jan 30, 2020

Has anyone used optionalDependencies before? The name of them sounds like what I want, but the description in the NPM docs makes it sound like it will still try to install them?

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object. This is a map of package name to version or url, just like the dependencies object. The difference is that build failures do not cause installation to fail.

What I want is NPM install to warn if the user's installed version does not match the version range, to give a warning, but if they haven't specified it, do nothing. I'll handle checking for their existence and throwing an error if the required one for the chosen renderer is missing.

Unfortunately I cannot test it without publishing something and seeing how it behaves, which is lot of effort for a maybe.

@kamranayub
Copy link

I didn't find this until a few minutes ago but I wanted to mention I spent all day trying to debug why a simple test was throwing a Node out-of-memory exception when I was testing useQuery hook from react-query with RTL Hooks.

Sometimes I was able to see the underlying error:

console.error
    Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.
        in TestHook
        in Suspense

But mostly it would hang Jest until Node spit out its GC allocation error and report JSON.

Really threw me for a loop!

Turns out that it was a combination of a couple things:

  1. In our test runner, we had set process.browser = true in setup even when using node jest environment, so browser-only code was running (we're using that across our packages instead of checking window)
  2. Trying to create a minimal repro, I took out the initialData option when testing, which "reproduced" the issue in react-query minimally but was a red herring since RTL was executing hooks when it shouldn't have.

These two issues led to me running around trying to figure out why react-query was broken and only now did I realize that I shouldn't/can't use RTL for testing SSR hooks 😢 Once I switched to react-dom/server (and I fixed setting process.browser) it worked like I hoped.

@mpeyper
Copy link
Member

mpeyper commented Apr 21, 2020

Sorry this hasn't moved. I've been very busy in my "real job" and evenings have become "me time" for a bit.

I'm feeling like I'm between a rock and a hard place on this one. I really don't want to introduce additional peerDependenvies but I also don't particularly want to publish multiple packages and choose which tradeoff I hate the least has me paralyzed on it.

@mpeyper
Copy link
Member

mpeyper commented Jun 5, 2020

I've recently been informed about peerDependenciesMeta (thanks @eps1lon) which is supported by all the major package managers now, and is ideal for my purposes here.

Given that, I'm going to be looking at this feature again and should hopefully have a PR up in the next few days (come bug me if it's not there in a week).

@joshuaellis
Copy link
Member

I'm a bit unsure why this issue is open if the PR was closed, I'm happy to take a look if I could get an overview on what's going on, worried the comments are a bit outdated.

@mpeyper
Copy link
Member

mpeyper commented Dec 3, 2020

Hi @joshuaellis,

The issue is open because the PR was not merged. It was more of a POC that anything else and I lost traction on it a while ago.

There has been some other discussion around another proposal that is easier to implement that indicates it may be a suitable a solution to testing SSR as well.

Ultimately, I think the comments here are still valid, it's just the effort that has gone stale. I'm still ok with the approach in the PR, I'm just not sure about the long term maintenance of supporting different renderers is something we want to take on. Potentially something like @simmo's original suggestion could be easier to implement and maintain in the long run, at the expense of features and flexibility.

@kettanaito
Copy link

kettanaito commented Dec 3, 2020

What do you think about encapsulating the right renderer/acting package within the renderHook and act?

import { renderHookServer } from './internal/renderHookServer'
import { renderHookNative } from './internal/renderHookNative'

function renderHook(...) {
  if (IS_SSR) {
    // Render using "react-dom/server"
   return renderHookServer(...)
  }

  if (IS_NATIVE) {
    // Render using "react-testing-utils"
    return renderHookNative(...)
  }

  // Otherwise, assume we're in the DOM.
  return renderHookDOM(...)
}

We can determine the right dependencies based on the environment renderHook is being run in. I know this increases the complexity of the renderHook function, yet we can split the functionality internally into renderHookServer and alike, without having to expose them publicly, or create separate packages.

@mpeyper
Copy link
Member

mpeyper commented Dec 3, 2020

@kettanaito that's pretty similar to what I was doing to autodetect which version to use in #387, only the variations were not considered internal.

It's also not possible (as far as I'm aware) to detect if someone wants to use server rendering or not when rendering their hook as the environment the tests run in doesn't change in any of the three scenarios (most users are just running jest these days).

I also want to avoid is forcing people to install dependencies they don't to support renderers they aren't using.

@joshuaellis
Copy link
Member

joshuaellis commented Dec 3, 2020

Hey @mpeyper sorry for the late response, there's a lot to read here and i've been trying to find time all day to give it the time it deserves. I've looking at the (now closed) PR and i'm just wondering why you felt the need to create seperate ends for dom|native|server when server is the only one that requires a different renderer? This would reduce an overhead imo – maybe I missed a comment or something?

In regards to the other proposal I'm not sure would work for a test like @simmo's here where the error won't be thrown because window is still available. Although it's something we can investigate when #496 is merged.

@mpeyper
Copy link
Member

mpeyper commented Dec 3, 2020

While you're correct that server is the only one that requires a different renderer, the dom and native functionality was split to utilise the dependencies people are already using.

If they are developing a web app they will likely have react-dom installed and so installing react-test-renderer is unnecessary , especially if they are using @testing-library/react for testing their actual components. If they also want to test SSR they will also have to ensure both dependencies are available.

Meanwhile, react-native users have no reason to install react-dom and will already be using react-test-renderer or @testing-library/react-native (which uses react-test-renderer) for testing their components. It is also less likely they will have a DOM set up in their test environment so removing that setup into their project is also preferable.

So there are 3 use cases that use 3 different renderers:

  1. DOM - react-dom
  2. Server - react-dom/server
  3. Native - react-test-renderer

The closed PR had a root entry that would try to autodetect between dom and native by checking which renderer dependencies were installed to reduce the migration friction and generally make it easier for people that didn't care, but for those that wanted to test their hook for specific use cases could be selective and use the explicit imports. I also wondered if anyone bundling their test suite to run in an actual browser would have issues with the auto import method and could use the explicit imports as a fallback for those scenarios.

@mpeyper
Copy link
Member

mpeyper commented Dec 3, 2020

I'll also add that the PR implementation made it possible to provide complete custom renders, so things like @testing-library/preact-hooks could migrate to using the common hook wrangling logic and provide the preact renderer instead.

Theoretically any react renderer would be supportable.

@mpeyper
Copy link
Member

mpeyper commented Dec 3, 2020

Just looking at that list of renderers I posted, this caught my eye.

I appears to also have a test server renderer so might be able to support all three use cases with a single dependency. There is a warning in the README that it's not intended to actually be used for things and isn't documents at all so it's probably a fairly risky option, but could be worth investigating?

Edit: Just looking through the code of noop-renderer, I think it will log out a bunch of stuff that we don't want/need.

@joshuaellis
Copy link
Member

I think realistically the solution you previously impliments makes the most sense, it also mimics how people will be rendering their software which gives added confidence. Althought there are three renderers, there's not a lot too them imo for maintance. There may be some more abstractions inside the customRenderers we could potentially do to avoid duplication, but i'm not experienced enough in bundlers to know if that would cause issues.

Theoretically any react renderer would be supportable.

I think this is really powerful and a definite advantage to that approach too.

If you're happy, I can gather the bits from the old PR and carry on where you left off? In the future we may be able to unify renderers, i.e if react-test-renderer does SSR.

@joshuaellis
Copy link
Member

Also because this is a large change we could do a release candidate / beta / alpha (whatever you want to call it). That way people can test the SSR implimentation to ensure it's as useful as we want it to be.

@mpeyper
Copy link
Member

mpeyper commented Dec 4, 2020

If you're happy, I can gather the bits from the old PR and carry on where you left off?

Absolutely! I'm not the sort that is too precious of my own code so feel free to hack and slash it as much as you want.

@mpeyper
Copy link
Member

mpeyper commented Jan 7, 2021

#510 has been released as 5.0.0-beta.1. There's no documentation yet, but the short version is that all these should work:

  • import { renderHook, act } from '@testing-library/react-hooks'
  • import { renderHook, act } from '@testing-library/react-hooks/pure'
  • import { renderHook, act } from '@testing-library/react-hooks/dom'
  • import { renderHook, act } from '@testing-library/react-hooks/dom/pure'
  • import { renderHook, act } from '@testing-library/react-hooks/native'
  • import { renderHook, act } from '@testing-library/react-hooks/native/pure'
  • import { renderHook, act } from '@testing-library/react-hooks/server'
  • import { renderHook, act } from '@testing-library/react-hooks/server/pure'

Let us know if you have any issues.

@joshuaellis joshuaellis removed the help wanted Extra attention is needed label Jan 7, 2021
@joshuaellis joshuaellis mentioned this issue Jan 8, 2021
2 tasks
@joshuaellis
Copy link
Member

This is released in v5.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released on @beta request for comment Discussion about changes to the library
Projects
None yet
Development

No branches or pull requests

6 participants