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

Update custom server examples #24814

Merged
merged 5 commits into from
May 7, 2021

Conversation

timneutkens
Copy link
Member

@timneutkens timneutkens commented May 5, 2021

Fixes the links/readme descriptions, also removes legacy routing that can now be handled using rewrites which also work client-side automatically.

Closes #24607

Bug

  • Related issues linked using fixes #number
  • Integration tests added

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.

Documentation / Examples

  • Make sure the linting passes

@ijjk ijjk added the examples Issue was opened via the examples template. label May 5, 2021
@timneutkens timneutkens changed the title fix/custom server example Update custom server examples May 5, 2021
server.get('/b', (req, res) => {
return app.render(req, res, '/b', req.query)
})

server.all('*', (req, res) => {
return handle(req, res)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As someone trying to understand how the Next.js custom Express setup works, I have some questions:

If the server.get() path handlers are not needed here, does that mean the server.all() handler will just know how to handle all the paths defined in pages? So I'm assuming we can just define API endpoints here that Express needs to handle and all the application path routing will just work.

If yes, then what's the point of all the app.render in the documentation example? Shouldn't that only be done when file-system routing is disabled? Having it in the basic example makes it look like it's necessary to define all the path handlers.

I'm also confused about what the query parameter is and how and when it is supposed to be used. In the lines deleted here, we were using req.query and that worked fine. But the documentation example shows that query should be taken from the result of parse(req.url, true) and then passed to the app.render. Can't find any explanation of what query parameter is exactly supposed to be and why does app.render need it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, my use case is that we have a fat Express server serving the build folder from create-react-app. Trying to figure out how to migrate to Next.js to do server-rendering.

I'd expect the documentation/examples to have a mention of:

  • Where do the express middlewares fit here?
  • Is it necessary to define path handlers for every route of the application?
  • Where are the response headers supposed to be in a custom server with Next.js? Will express keep handling the headers for every page request or will Next.js headers override that functionality? Are there any conflicts between the two?

Understandably, these questions could be directed to Discussions or StackOverflow but it'd be great if it's possible to figure these answers out from the documentation.

@timneutkens timneutkens merged commit 9506f15 into vercel:canary May 7, 2021
@timneutkens timneutkens deleted the fix/custom-server-example branch May 7, 2021 16:50
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Jun 1, 2021
@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.
Labels
examples Issue was opened via the examples template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants