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

Hapi example broken: inconsistent server promises #1170

Closed
fionawhim opened this issue Feb 16, 2017 · 4 comments
Closed

Hapi example broken: inconsistent server promises #1170

fionawhim opened this issue Feb 16, 2017 · 4 comments

Comments

@fionawhim
Copy link
Contributor

It looks like #1099 left Hapi in a broken state with main.js and common.js modules. The various render* methods in server/index.js are inconsistent about whether they return a promise, so what’s happening now is that the defaultHandlerWrapper is closing the response before the renderStatic method actually renders anything, since it returns void without any await.

Note the ”Content-Length: 0“
screen shot 2017-02-15 at 8 08 49 pm

My proposed fix for this, which I can put a PR together for, is to make sure (and document and/or test) that all the methods of Server are consistently returning promises that don’t resolve until they have written their complete output.
@rauchg
Copy link
Member

rauchg commented Feb 16, 2017

Sounds good. Apologize for merging a bad example. My knowledge of Hapi is limited

@rauchg
Copy link
Member

rauchg commented Feb 16, 2017

@finneganh I added you as a collaborator in case you want to own all the Hapi-related PRs

@fionawhim
Copy link
Contributor Author

I'm not much of an expert but I'm willing to take a look at them.

fionawhim pushed a commit to fionawhim/next.js that referenced this issue Feb 16, 2017
Async render fixes for vercel#1170
7cf3ae4 
The change in vercel#1155 to remove server-side gzipping changed static
rendering to no longer return a promise, which broke the Hapi example
that was waiting for a resolved promise before closing the request.

This PR fixes up all render or serve methods of server.js to
consistently either await or return. Additionally, it collapses
serveStatic and _serveStatic, as _serveStatic no longer needs to be
factored out.
fionawhim pushed a commit to fionawhim/next.js that referenced this issue Feb 16, 2017
Async render fixes for vercel#1170
6e9f9dd 
The change in vercel#1155 to remove server-side gzipping changed static
rendering to no longer return a promise, which broke the Hapi example
that was waiting for a resolved promise before closing the request.

This PR fixes up all render or serve methods of server.js to
consistently either await and/or return. Additionally, it collapses
serveStatic and _serveStatic, as _serveStatic no longer needs to be
factored out.
@fionawhim
Copy link
Contributor Author

@rauchg Looks like the Hapi change wasn't itself the problem, it was the Hapi change followed by the removal of Gzip.

Fix available in #1172, which tries to be defensive against async things not being captured going forward.

rauchg pushed a commit that referenced this issue Feb 16, 2017
@fionawhim @rauchg
Async render fixes for #1170 (#1172)
47d57f4 
The change in #1155 to remove server-side gzipping changed static
rendering to no longer return a promise, which broke the Hapi example
that was waiting for a resolved promise before closing the request.

This PR fixes up all render or serve methods of server.js to
consistently either await and/or return. Additionally, it collapses
serveStatic and _serveStatic, as _serveStatic no longer needs to be
factored out.
@lock lock bot locked as resolved and limited conversation to collaborators May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rauchg @fionawhim @timneutkens