Handle accept headers totally inside Next.js #385

arunoda · 2016-12-13T13:12:52Z

Now user doesn't need to handle accept headers anymore. So the code looks like this:

const express = require('express')
const next = require('next')

const app = next({ dir: '.', dev: true })
const handle = app.getRequestHandler()

app.prepare()
  .then(() => {
    const server = express()

    server.get('/a', (req, res) => {
      return app.render(req, res, '/b', req.query)
    })

    server.get('/b', (req, res) => {
      return app.render(req, res, '/a', req.query)
    })

    server.get('*', (req, res) => {
      return handle(req, res)
    })

    server.listen(3000, (err) => {
      if (err) throw err
      console.log('> Ready on http://localhost:3000')
    })
  })

So now users doesn't need to know anything about accept headers and routing looks so simple.

Now user doesn't need to handle it anymore.

coveralls · 2016-12-13T13:15:31Z

Coverage decreased (-0.07%) to 48.163% when pulling 49cc28e on arunoda:simple-customizable-api into 8318728 on zeit:add/programmatic-api.

arunoda · 2016-12-13T13:15:51Z

server/index.js

+    if (accept.type(['json', 'html']) === 'json') {
+      // When we are asking for the page json, we always need to give
+      // the real pathname. But not the pathname user provided.
+      const { pathname: realPathname } = parse(req.url)


Now we've to parse the url twice to get the pathname. But we don't need query params just like in the other case.

This might add some overhead but it's negligible because of following reasons:

Now we don't parse accept headers twice

Rendering the React component on the server is so much costly than this.

arunoda · 2016-12-13T13:16:18Z

@rauchg @nkzawa what do you guys think of this?

arunoda · 2016-12-13T13:16:55Z

server/index.js

@@ -62,12 +62,7 @@ export default class Server {

    this.router.get('/:path*', async (req, res) => {
      const { pathname, query } = parse(req.url, true)
-      const accept = accepts(req)


Now we don't need to parse accept headers twice. We only do it once.

nkzawa · 2016-12-13T13:32:11Z

I also was considering this way which I think is a valid option but has some problems.

app.render() becomes too magical
have to check the accept header anyway when you want to use app.renderToHTML() on your application.

arunoda · 2016-12-13T13:38:18Z

app.render() becomes too magical

It isn't too much magical. We need to document what's its for inside our code.
This is something the framework does. And user has no way to alter serving JSON pages.
So, there's no good reason to let user to handle it.

May be going back to .json instead of accept headers is another alternative.
BTW: Why did we move to accept headers?

have to check the accept header anyway when you call app.renderToHTML()

Yes it is. But that might not be general use of the public API. If the user wanna do that, he know what's he is going. Most of the user may don't need to use it. (If that's not the case correct me)

nkzawa · 2016-12-13T14:00:24Z

BTW: Why did we move to accept headers?

Because it's generally a nicer way. I think we can change it if we have valid reasons.

Yes it is. But that might not be general use of the public API. If the user wanna do that, he know what's he is going. Most of the user may don't need to use it. (If that's not the case correct me)

A possible use case of renderToHTML is caching SSR results. Another is customizing response headers. Maybe it's not rare.

I think API would be cleaner if the difference of render and renderToHTML was only how they handle rendering results. It's arguable though.

nkzawa · 2016-12-13T14:05:04Z

examples/custom-server-express/server.js

-      return handle(req, res)
-    }
-    next()
-  })


I was rather thinking how to make handling JSON easier, for example, we can support a middleware-like API.

server.use(app.handleJSON())

If server is not express:

function requestHandler (req, res) { app.handleJSON(req, res, (err) => { // called only if Accept header is not JSON. }) }

I think this is something pretty great.
I'll change the API to be like this.

Anyway, how about changing it back to .json. Accept headers won't give us any advantages but problems.

With this way, we could keep actual URLs and page JSON separately. Actually, that's how they work.

coveralls · 2016-12-15T00:12:39Z

Coverage increased (+0.005%) to 48.234% when pulling 281394f on arunoda:simple-customizable-api into 8318728 on zeit:add/programmatic-api.

nkzawa · 2016-12-15T02:13:31Z

server/index.js

@@ -50,6 +49,12 @@ export default class Server {
      await this.serveStatic(req, res, p)
    })

+    this.router.get('/_next/pages/:path*', async (req, res, params) => {
+      const path = params.path || ['']


I think this has to be params.path.join('/') when :path* is nested.

Okay. Good call.

coveralls · 2016-12-15T04:20:41Z

Coverage increased (+0.005%) to 48.234% when pulling d67b1f4 on arunoda:simple-customizable-api into 8318728 on zeit:add/programmatic-api.

rauchg · 2016-12-15T19:07:06Z

So cool

* add 'next' api * add render APIs * add 'as' prop to Link * check Accept header to serve json response * check if response was finished on getInitialProps call * move server/app to server/index * load webpack-hot-middleware-client by absolute path * server: options for testing * add tests * example: improve * server: make dir optional * fix client routing * add parameterized routing example * link: fix display url * Add custom-server-express example (#352) * Add custom-server-express example * Remove extraneous nexts in express routes defs * Update next config in server.js * Handle accept headers totally inside Next.js (#385) * Handle accept headers totally inside Next.js Now user doesn't need to handle it anymore. * Move json pages serving to /_next/pages base path. * Join paths correctly. * remove next/render

Handle accept headers totally inside Next.js

49cc28e

Now user doesn't need to handle it anymore.

arunoda commented Dec 13, 2016

View reviewed changes

nkzawa reviewed Dec 13, 2016

View reviewed changes

Move json pages serving to /_next/pages base path.

281394f

nkzawa reviewed Dec 15, 2016

View reviewed changes

Join paths correctly.

d67b1f4

rauchg merged commit 4508725 into vercel:add/programmatic-api Dec 15, 2016

arunoda deleted the simple-customizable-api branch December 15, 2016 22:54

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle accept headers totally inside Next.js #385

Handle accept headers totally inside Next.js #385

arunoda commented Dec 13, 2016 •

edited

Loading

coveralls commented Dec 13, 2016 •

edited

Loading

arunoda Dec 13, 2016

arunoda commented Dec 13, 2016

arunoda Dec 13, 2016

nkzawa commented Dec 13, 2016 •

edited

Loading

arunoda commented Dec 13, 2016 •

edited

Loading

nkzawa commented Dec 13, 2016

nkzawa Dec 13, 2016 •

edited

Loading

arunoda Dec 13, 2016

coveralls commented Dec 15, 2016 •

edited

Loading

nkzawa Dec 15, 2016

arunoda Dec 15, 2016

arunoda Dec 15, 2016

coveralls commented Dec 15, 2016 •

edited

Loading

rauchg commented Dec 15, 2016

Handle accept headers totally inside Next.js #385

Handle accept headers totally inside Next.js #385

Conversation

arunoda commented Dec 13, 2016 • edited Loading

coveralls commented Dec 13, 2016 • edited Loading

arunoda Dec 13, 2016

Choose a reason for hiding this comment

arunoda commented Dec 13, 2016

arunoda Dec 13, 2016

Choose a reason for hiding this comment

nkzawa commented Dec 13, 2016 • edited Loading

arunoda commented Dec 13, 2016 • edited Loading

nkzawa commented Dec 13, 2016

nkzawa Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

arunoda Dec 13, 2016

Choose a reason for hiding this comment

coveralls commented Dec 15, 2016 • edited Loading

nkzawa Dec 15, 2016

Choose a reason for hiding this comment

arunoda Dec 15, 2016

Choose a reason for hiding this comment

arunoda Dec 15, 2016

Choose a reason for hiding this comment

coveralls commented Dec 15, 2016 • edited Loading

rauchg commented Dec 15, 2016

arunoda commented Dec 13, 2016 •

edited

Loading

coveralls commented Dec 13, 2016 •

edited

Loading

nkzawa commented Dec 13, 2016 •

edited

Loading

arunoda commented Dec 13, 2016 •

edited

Loading

nkzawa Dec 13, 2016 •

edited

Loading

coveralls commented Dec 15, 2016 •

edited

Loading

coveralls commented Dec 15, 2016 •

edited

Loading