fridays / next-routes

Universal dynamic routes for Next.js
MIT License
2.47k stars 230 forks source link

Integration with ssr-caching #4

Closed sedubois closed 7 years ago

sedubois commented 7 years ago

Next.js provides a great SSR caching example which makes use of Next.js' renderToHTML and renderError. Can next-routes somehow be used in conjunction with this or could this be implemented?

https://github.com/zeit/next.js/tree/master/examples/ssr-caching

fridays commented 7 years ago

I didn't try it yet, but it should work like this with the caching example:

const express = require('express')
const next = require('next')
const routes = require('./routes')
const dev = process.env.NODE_ENV !== 'production'
const app = next({ dir: '.', dev })
const handle = app.getRequestHandler()
const LRUCache = require('lru-cache')

const ssrCache = new LRUCache({
  max: 100,
  maxAge: 1000 * 60 * 60 // 1hour
})

app.prepare().then(() => {
  express().use(renderAndCache).listen(3000)
})

function renderAndCache (req, res) {
  if (ssrCache.has(req.url)) {
    return res.send(ssrCache.get(req.url))
  }

  // Match route + parse params
  const {route, params} = routes.match(req.url)
  if (!route) return handle(req, res)

  app.renderToHTML(req, res, route.page, params).then((html) => {
    ssrCache.set(req.url, html)
    res.send(html)
  })
  .catch((err) => {
    app.renderError(err, req, res, route.page, params)
  })
}
sedubois commented 7 years ago

Ah nice, will try 🙂 It might be helpful to document in README the routes.match() API.

fridays commented 7 years ago

I'll do that! Let me know if you encounter any issues. Closing this for now

sedubois commented 7 years ago

I tried in this branch: https://github.com/relatenow/relate/tree/ssr-caching

But I get in the server:

CACHE MISS: /__webpack_hmr
CACHE MISS: /_next-prefetcher.js
CACHE HIT: /__webpack_hmr

And in the console: EventSource's response has a MIME type ("text/html") that is not "text/event-stream". Aborting the connection.

The __webpack_hmr requests shouldn't hit the cache I guess, but don't know how to configure that using next-routes. Any help appreciated :)

sedubois commented 7 years ago

I tried adding this before checking the cache:

    if (req.url === '/__webpack_hmr') {
      return handle(req, res);
    }    

But still get

The script has an unsupported MIME type ('text/html').
Failed to load resource: net::ERR_INSECURE_RESPONSE
sedubois commented 7 years ago

I also think that somehow, the route('profile', '/:slug') shouldn't match on /__webpack_hmr. /__webpack_hmr shouldn't match on any user route.

sedubois commented 7 years ago

OK for some reason the console error doesn't appear any more (although I thought I had restarted the server to clear the cache).

The next issue is that it doesn't match on all the prefetcher routes, although this is the most important load that the server will get (most of the time, the server will only need to render when the user visits the home page, then the rest of SSR will only be triggered by the SSR requests):

Route not matched: /static/nprogress.css
Route not matched: /_next/-/commons.js
Route not matched: /_next/-/main.js
Route not matched: /_next/-/pages/
Route not matched: /_next/-/pages/discover
Route not matched: /_next/-/pages/about
Route not matched: /_next/-/pages/auth/login
Route not matched: /static/homepage/bg_TimMarshall.jpg
sedubois commented 7 years ago

I asked about this issue in the original PR: https://github.com/zeit/next.js/pull/497#issuecomment-279661040

sedubois commented 7 years ago

And I replied to myself 😄

Oh I keep forgetting, next-prefetcher doesn't do a pre-render, it just downloads the code.

fridays commented 7 years ago

route('profile', '/:slug') shouldn't match on /__webpack_hmr

It doesn't since the catchall update from the last version, can you check?

I just updated /_next/ to /_next in the ignored paths, so prefetcher urls won't be matched by catchall routes anymore too.

Also you can now use routes.isNextPath(req.url) to test against the list of ignored paths. Maybe that can be helpful when integrating with the caching system.

sedubois commented 7 years ago

It doesn't since the catchall update from the last version, can you check?

@fridays created https://github.com/fridays/next-routes/issues/6

Also you can now use routes.isNextPath(req.url)

Seems to work 👍 (https://github.com/relatenow/relate/commit/f8032ba752d8d45d7d5ceebc8412b8118614f465#diff-bba6db1dd9623c6662b43cc660a5975cR20)