-
Notifications
You must be signed in to change notification settings - Fork 232
Server side rendering #68
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
Comments
Looks cool. I wonder how this would feel as an option to My other thought is that we recently changed to use |
Hey @mpeyper I'm thinking out-loud here... I was wondering if we could flag And, yes, you raise a valid point about hydration. For example you'd maybe expect a I'll have a play with |
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? |
Closing this due to inactivity. Happy to reopen if anyone wants to start the discussion back up. |
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. |
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. |
On SSR, |
Of course, yes, that's a thing, isn't it... So rendering with 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? |
So I've briefly looked into it, and there does not appear to be a way to use So I see three scenarios:
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. |
For my tests I don‘t really care for hydration, SSR-only and CSR-only suffices for my needs. |
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. |
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! |
The |
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
The way I currently have it is that if you don't specify The other issue with what I currently have is that it has a
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.
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. |
Using the same name |
I don't think the naming of If they were named differently, then there would be no reason to have the This looks kind of ok for Another option is to only seperate
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 |
Has anyone used
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. |
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 Sometimes I was able to see the underlying error:
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:
These two issues led to me running around trying to figure out why |
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 |
I've recently been informed about 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). |
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. |
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. |
What do you think about encapsulating the right renderer/acting package within the 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 |
@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. |
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 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 |
While you're correct that If they are developing a web app they will likely have Meanwhile, So there are 3 use cases that use 3 different renderers:
The closed PR had a root entry that would try to autodetect between |
I'll also add that the PR implementation made it possible to provide complete custom renders, so things like Theoretically any react renderer would be supportable. |
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 |
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.
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 |
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. |
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. |
#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:
Let us know if you have any issues. |
This is released in v5.0.0 |
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! 😄
The text was updated successfully, but these errors were encountered: