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.

Sign up for GitHub

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

Jump to bottom

renderToHTML with getServerSideProps send response instead of return html from 9.3.1 #11665

Closed
superkoh opened this issue Apr 4, 2020 · 16 comments
Closed

renderToHTML with getServerSideProps send response instead of return html from 9.3.1 #11665

superkoh opened this issue Apr 4, 2020 · 16 comments
Labels
please add a complete reproduction Please add a complete reproduction.

Comments

@superkoh
Copy link

superkoh commented Apr 4, 2020

Bug report

Describe the bug

when upgrade from 9.3.0 to higher version, pages with renderToHtml direct send response instead of html. it work correct when using getInitialProps.

Expected behavior

rendertohtml should always return html instead of send response
@timneutkens
Copy link
Member

A complete reproduction is missing, please provide one.

@timneutkens timneutkens added the please add a complete reproduction Please add a complete reproduction. label Apr 5, 2020
@ngdnghia28
Copy link

related to: #11525

@visum
Copy link

visum commented Apr 7, 2020

Reproduction repo: https://github.com/visum/next-renderToHTML-test

@superkoh
Copy link
Author

@timneutkens , as @visum provided. i have the same issue like him

@HaNdTriX HaNdTriX mentioned this issue Apr 19, 2020
Closed
@leanazulyoro
Copy link

The problem as stated in the title is that the method renderToHtml is sending the response, not just rendering the HTML. I agree that the method should return the html, ence it's name.
In the meantime, I ask:
https://github.com/zeit/next.js/blob/canary/packages/next/next-server/server/next-server.ts#L1009-L1015
What if the method returns the html after it's sent? we would need to remove the line 1014 in next-server.js. I tryed that and it works good enought to cache the response and make this work again

@na-ji
Copy link

na-ji commented Apr 28, 2020

In case it's not clear, another complete reproduction: https://github.com/zeit/next.js/tree/canary/examples/ssr-caching

kodiakhq bot pushed a commit that referenced this issue May 22, 2020
@xliusstk
Fix ssr-caching example (#12180)
6c5efd7 
cacheable-response.get expects a get(req, res) method signature:
https://www.npmjs.com/package/cacheable-response#get

Before the change:
```
curl -s  -v http://localhost:3000/blog/first 2>&1 | grep HTTP/1.1
> GET /blog/first HTTP/1.1
< HTTP/1.1 404 Not Found
```
After the change:
```
curl -s  -v http://localhost:3000/blog/first 2>&1 | grep HTTP/1.1
> GET /blog/first HTTP/1.1
< HTTP/1.1 200 OK
```

This is a partial fix of #12019 to make the example work. However it doesn't fix the error 
```
(node:62360) UnhandledPromiseRejectionWarning: Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
``` , which will need a change at server/next-server.ts

See related issue:
#11525 
#11665
@visum
Copy link

visum commented Jun 26, 2020

The problem appears to be in renderToHTMLWithComponents, which has a number of cases where it calls sendPayload instead of returning the HTML. There are a lot of cases in there and I'm having a hard time sorting through them all, but adding getServerSideProps to a page apparently triggers one of those cases.

Given that renderToHTML depends on renderToHTMLWithComponents, seems like the latter should always return HTML and let its caller handle sending the response. The name renderToHTMLWithComponents wouldn't suggest to me that it does anything more than render the HTML and return it.

@Timer
Copy link
Member

Timer commented Jun 29, 2020

This is the expected behavior, as pages powered by getServerSideProps must control the entire request lifecycle, including caching headers. This is necessary to prevent user-specific information from leaking into the application. Please open an issue for the specific use case you're looking to accomplish by getting the HTML response!

@Timer Timer closed this as completed Jun 29, 2020
@visum
Copy link

visum commented Jun 30, 2020 •
edited
Loading

It makes sense that a page using getServerSideProps needs more control over the request lifecycle, and in fact setting caching headers is one of the reasons I use it. However, I'm not following how that influences where Next.js sends the response back. The sending of the response is the part of the lifecycle that the page itself doesn't handle.

The specific use case is one that the existing documentation and examples seem to indicate should be possible using renderToHTML - a custom caching mechanism. If I ask Next.js for the HTML and it sends the response (and doesn't give me the HTML), I don't have an opportunity to cache the page. For a page using getServerSideProps because it can't be statically optimized, this is especially important.

At the very least, the API could make more sense. renderToHTML sounds like it should return HTML.

@Timer
Copy link
Member

Timer commented Jun 30, 2020 •
edited
Loading

renderToHTML is a private API that's not documented, however, we knew some users were relying on it so we didn't break any existing behavior (with getInitialProps). The new lifecycles cannot return the HTML for further processing.

I've opened an issue to remove all usage of renderToHTML from our examples:
#14737

These examples will be updated with the appropriate (public API) approaches.

Reiterating above:

Please open an issue for the specific use case you're looking to accomplish by getting the HTML response! We're happy to entertain use cases.

@P233
Copy link

P233 commented Jul 1, 2020

@Timer My current project heavily relies on the renderToHTML method to cache the dynamically generated pages which grab data from both a headless CMS and a regular database. The caches live 24 hours to speed up the loading speed and reduce the number of API requests. It works pretty well, but we have to stay with the next version 9.3.0. Please provide an alternative recommended way for SSR caching :)

@ycscholes
Copy link

@Timer My current project heavily relies on the renderToHTML method to cache the dynamically generated pages which grab data from both a headless CMS and a regular database. The caches live 24 hours to speed up the loading speed and reduce the number of API requests. It works pretty well, but we have to stay with the next version 9.3.0. Please provide an alternative recommended way for SSR caching :)

So does our project

@P233
Copy link

P233 commented Jul 2, 2020

After carefully reading the RFC: Incremental Static Regeneration, I believe we will switch to the Incremental Static Regeneration solution later.

Sorry for polluting this thread!

@jack-pallot
Copy link

If renderToHTML potentially breaks existing functionality, or isn't the intended behaviour of the function, then it might be better to create a new one like getStaticHTML instead specifically for this use case. It's unfortunate that the renderToHTML name is already taken, but if it's no longer in example code it shouldn't cause confusion going forward.

rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
@xliusstk @rokinsky
Fix ssr-caching example (vercel#12180)
5cc0b1a 
cacheable-response.get expects a get(req, res) method signature:
https://www.npmjs.com/package/cacheable-response#get

Before the change:
```
curl -s  -v http://localhost:3000/blog/first 2>&1 | grep HTTP/1.1
> GET /blog/first HTTP/1.1
< HTTP/1.1 404 Not Found
```
After the change:
```
curl -s  -v http://localhost:3000/blog/first 2>&1 | grep HTTP/1.1
> GET /blog/first HTTP/1.1
< HTTP/1.1 200 OK
```

This is a partial fix of vercel#12019 to make the example work. However it doesn't fix the error 
```
(node:62360) UnhandledPromiseRejectionWarning: Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
``` , which will need a change at server/next-server.ts

See related issue:
vercel#11525 
vercel#11665
@jorge07
Copy link

jorge07 commented Mar 26, 2021 •
edited
Loading

I've a use case that I can't resolve with the current implementation.
I've a 3rd party app I need to call for some reasons and set some cookie before reply to the user and I've to do it for all pages.
Because I don't want to block my app I want to do this request in parallel while rendering the next app and wait until both are resolved to send the response to the user. Unfortunately because we use in some pages the server side props (most of them) this isn't working as expected and the API provided is confusing as res.end doesn't sound like something you expect when calling renderToHTML.
Is is possible to just access to the generate HTML code?
I know I break some functionalities but I will never use in this app.

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
please add a complete reproduction Please add a complete reproduction.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests