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

[RFC] server(less) middleware #7208

Closed
timneutkens opened this issue May 1, 2019 · 55 comments · Fixed by #12218
Closed

[RFC] server(less) middleware #7208

timneutkens opened this issue May 1, 2019 · 55 comments · Fixed by #12218

Comments

@timneutkens
Copy link
Member

timneutkens commented May 1, 2019

Feature request

Currently there are a lot of users that create a custom server to do middleware behavior, for example server-side parsing of request bodies, cookies etc. Currently Next.js provides no way of handling this without a custom server. Meaning it can't be done in serverless / other environments.

Describe the solution you'd like

Ideally users would define middleware as a top level export. Meaning that pages can define middleware requirements:

// PageContext being the same values as `getInitialProps`'s context
export async function middleware({req, res}: PageContext) {
  // do something with `req` / `res`
}

function IndexPage() {
  return <h1>Hello world</h1>
}

export default IndexPage

However this also introduces the complexity of having to code-split / tree shake away user imports that are server-only.

So for the initial implementation (and incrementally working towards the implementation above) I'd start with supporting middleware in pages_document.js which is always server-side rendered so it makes sense to have it there.

One thing that was brought up is "why can't this just be done in getInitialProps of _document".

The reason that we need a new method is that the lifecycle of calling getInitialProps looks like this:

  • pages/_app.js getInitialProps is called,
  • pages/_app.js getInitialProps calls the page's getInitialProps
  • pages/_document.js getInitialProps is called
  • pages/_document.js getInitialProps calls renderPage
  • renderPage calls React's renderToString and returns the html, head tags and styles.
  • renderToStaticMarkup is called to render the _document shell
  • request is ended using send-html.ts, which adds etag etc.

Generally when using middleware it has to be called before getInitialProps because you could be parsing something needed in getInitialProps

So the middleware has to be called earlier.

Meaning the lifecycle would look like:

  • pages/_document.js middleware is called
  • pages/_app.js getInitialProps is called,
  • pages/_app.js getInitialProps calls the page's getInitialProps
  • pages/_document.js getInitialProps is called
  • pages/_document.js getInitialProps calls renderPage
  • renderPage calls React's renderToString and returns the html, head tags and styles.
  • renderToStaticMarkup is called to render the _document shell
  • request is ended using send-html.ts, which adds etag etc.

So for the initial implementation we'd want something like:

// pages/_document.js

// PageContext being the same values as `getInitialProps`'s context
export async function middleware({req, res}: PageContext) {
  // do something with `req` / `res`
}

// rest of _document.js
@timneutkens timneutkens changed the title [RFC] server(less) middlware [RFC] server(less) middleware May 1, 2019
@timneutkens timneutkens added the RFC label May 1, 2019
@timneutkens
Copy link
Member Author

This new feature is aimed at supporting projects like next-i18next by default, which currently requires a custom server, cc @isaachinman

Relevant code: https://github.com/isaachinman/next-i18next/blob/master/src/middlewares/next-i18next-middleware.js#L52

@ivan-kleshnin
Copy link
Contributor

ivan-kleshnin commented May 2, 2019

What about React bugs that can be fixed ONLY by modifying the resulting HTML as a string?
I posted one earlier: #6718
I realize it raises additional concerns of streaming, etc. And it may be a rabbit hole. Nevertheless, let's at least discuss it to make us (consumers) aware of the official position at the moment. Should we have a pre and post stages or two different middlewares. Or it's just not worth it due to the extra complexity?

@isaachinman
Copy link
Contributor

@timneutkens As discussed via Slack - just want to clarify that we're talking about vanilla req and res here, and Express-specific fields like req.body and req.query will not be present. I imagine a lot of people are using Express (or similar) for middlewares, but what we're talking about here is a straight http implementation.

Is that correct?

@timneutkens
Copy link
Member Author

@isaachinman yep!

@ivan-kleshnin I don't understand how this relates, you'd be able to override res.end in a similar manner as what you're already doing in that issue, even though I wouldn't recommend it.

@Janpot
Copy link
Contributor

Janpot commented May 2, 2019

Just a quick thought and potentially bikeshedding, but would it make sense to use the Request and Response primitives from the fetch-API? Seems like it could be nice to be able to use the same primitives across server, serviceworker and frontend code.

@timneutkens
Copy link
Member Author

I'd rather stay with the Node.js req/res because:

  • It's what is passed to getInitialProps
  • It's compatible with most middlewares

@isaachinman
Copy link
Contributor

@timneutkens Agree with that completely. I was going to mention - there'd probably be room after this feature lands to churn out little packages to enable 1:1 replacement for things like Express. Much better to begin with Node defaults.

@jesstelford
Copy link
Contributor

♥️ it!

I've been using this workaround which allows me to add middlewares to getInitialProps():

See a demo of adding body-parser here: https://codesandbox.io/s/n3yj15mk7p

✅ Works for local dev/custom server (server.js)
✅ Works on target: 'serverless'
✅ Can parse anything (forms, JSON, etc)
✅ Is applied on a per-page basis
✅ Enables no-JS forms

@isaachinman
Copy link
Contributor

Hey @timneutkens and @Timer - great to see v9 has landed. I do believe that first-class support for API routes is a monumental and vital step for NextJs.

I've looked through #7297 and the new documentation, and it seems middleware support is not there yet, right? Would love to be able to rewrite next-i18next and remove the custom server requirement (i18next/next-i18next#274).

@trevordmiller
Copy link
Contributor

trevordmiller commented Aug 13, 2019

Our enterprise would also love to see this. We have an internal authentication system that requires a custom server currently and being able to move that logic to middleware and remove the custom server would dramatically simplify our project upgrading (we have hundreds of projects using Next - each major upgrade has had a decent amount of pain and pushback, other than next 9 - thanks for the backwards compat!).

@zomars
Copy link

zomars commented Aug 29, 2019

Just my two cents here: Wouldn't make sense to be able to add a _middleware.js in the pages/api/ directory?

@isaachinman
Copy link
Contributor

@zomars This is mostly relevant for actual frontend routes. It's already possible to use middlewares in your v9 API routes.

@amardeepsingh20
Copy link

@isaachinman Is there a fully working example you can refer to?

@hoangvvo
Copy link
Contributor

hoangvvo commented Sep 10, 2019

@amardeep9911 I might be wrong, but one way is to wrap the handler function.
https://github.com/zeit/next.js/tree/canary/examples/api-routes-middleware

@poupryc
Copy link

poupryc commented Oct 4, 2019

Hi @timneutkens, I'm testing the experimental functionality of middleware (great by the way) but I can never access the request object, when I log the middleware context I have this

> GET /                        
{                              
  err: undefined,              
  req: undefined,              
  res: undefined,              
  pathname: '/',               
  query: { amp: undefined },   
  asPath: '/',                 
  AppTree: [Function: AppTree] 
}                              

Should I create a separate issue? Or is that a normal restriction?

@game7
Copy link

game7 commented Oct 4, 2019

@HelloEdit I seem to remember that req/res are undefined unless your page has .getInitialProps() function, because without getInitialProps your page may be static

@poupryc
Copy link

poupryc commented Oct 4, 2019

ho, thank you for your answer. but I sincerely hope that it is not the expected behavior: the middleware would lose all their interest if getInitialProps was required to make them work, or I missed a point 🤔
My interpretation of this is that the middleware will always run on the server side when the request is made.

@callmenick
Copy link

@timneutkens nice, looking forward to when this gets implemented :) question for you. would this affect the automatic static optimizations outlined here?

@timneutkens
Copy link
Member Author

It wouldn't work with static pages.

@oeduardoal
Copy link

@mattcarlotta
Copy link
Contributor

mattcarlotta commented Jan 15, 2020

Would love to see a _middleware.js file that allow us add additional middleware to all req and res parameters (similar to express middlewares that are not page-specific). For example, a work around I've been using is to create a reusable middleware function that allows me to use compression, morgan, passport and cookie-sessions for all API requests:

import bodyParser from "body-parser";
import compression from "compression";
import morgan from "morgan";
import moment from "moment-timezone";
import passport from "passport";
import session from "cookie-session";
import applyMiddleware from "~middlewares/applyMiddleware";
import { sendError } from "~shared/helpers";

const { inProduction, cookieSecret } = process.env;

export default next => async (req, res) => {
	try {
		morgan.token("date", () => moment().format("MMMM Do YYYY, h:mm:ss a"));

		await applyMiddleware([
			compression({
				level: 6,
				filter: (req, res) =>
					req.headers["x-no-compression"]
						? false
						: compression.filter(req, res),
			}),
			session({
				path: "/",
				name: "app",
				maxAge: 2592000000, // 30 * 24 * 60 * 60 * 1000 expire after 30 days
				keys: [cookieSecret],
				httpOnly: true,
				// sameSite: inProduction, // specifies same-site cookie attribute enforcement
				// secure: inProduction,
			}),
			morgan(
				inProduction
					? ":remote-addr [:date] :referrer :method :url HTTP/:http-version :status :res[content-length]"
					: "tiny",
			),
			passport.initialize(),
			bodyParser.urlencoded({ extended: true }),
		])(req, res);

		return next(req, res);
	} catch (error) {
		return sendError(error, res);
	}
};

In addition, use an additional middleware function to check session authentication:

import get from "lodash/get";
import { User } from "~models/instances";
import { sendError } from "~shared/helpers";
import { badCredentials } from "~shared/errors";

/**
 * Middleware function to check if a user is logged into a session and the session is valid.
 *
 * @async
 * @function
 * @param {object} - req
 * @param {object} - res
 * @returns {function}
 */
export default next => async (req, res) => {
	const _id = get(req, ["session", "id"]);
	if (!_id) return sendError(badCredentials, res);

	const existingUser = await User.findOne({ _id });
	if (!existingUser) return sendError(badCredentials, res);

	next(req, res);
};

And then I manually wrap my API route(s) with them:

import withMiddleware from "~middlewares";
import { User } from "~models/instances";
import { sendError } from "~shared/helpers";
import requireAuth from "~strategies/requireAuth";

/**
 * Retrieves logged in user app settings.
 *
 * @async
 * @function getProfile
 * @param {object} - req
 * @param {object} - res
 * @returns {res}
 */
const getProfile = async (req, res) => {
	try {
		const { id: _id } = req.session;

		const signedinUser = await User.findOne({ _id }, { password: 0, __v: 0 });
		if (!signedinUser) throw String("Unable to locate signed in user profile.");

		res.status(200).json({ signedinUser });
	} catch (err) {
		return sendError(err, res);
	}
};

export default withMiddleware(requireAuth(getProfile));

While it works, the downside is that every API page function needs to be manually wrapped with this withMiddleware function. Worse, is that this creates a callback hell, where additional middlewares have to be wrapped (as shown above with requireAuth) and they have to manually pass req and res to the next function... and so on. By exposing a _middleware.js config, one could attach middlewares to a stack and apply them to all future requests/responses (ideally once, instead of for each request) and, as a result, this should clean up some of the API setup redundancy.

@usha-raikwar
Copy link

@timneutkens Wanted to check why this RFC is still open. The changes at #7209 has been merged into canary. When will this be merged into master and available to use under Next.js 9.

@timneutkens
Copy link
Member Author

It's unlikely this will land anytime soon as it introduces quite a bit of additional complexity for no additional gain based on upcoming features.

@usha-raikwar
Copy link

It's unlikely this will land anytime soon as it introduces quite a bit of additional complexity for no additional gain based on upcoming features.

Hope the team will suggest alternative for using middleware in client-side or if we can't use middleware at client-side at all!

Thanks!

@timneutkens
Copy link
Member Author

This proposal didn't cover client-side middleware.

@mattcarlotta
Copy link
Contributor

mattcarlotta commented Jan 23, 2020

@timneutkens The current canary implementation involves maintaining a _document page, however, since the middleware is primarily focused on modifying the IncomingMessage and ServerResponse properties... can we just modify them directly within the Server class instead?

For example, I have a working prototype that utilizes the Server's nextConfig property to include some middleware specified in an applyMiddleware property placed within the next.config.js file. This property defines an array of middleware functions and an optional routes configuration option -- it'll conditionally apply the middleware to specific routes -- and is structured like so:

next.config.js

const bodyParser = require("body-parser");
const morgan = require("morgan");
const passport = require("passport");
const session = require("cookie-session");

const { cookieSecret } = process.env;

module.exports = {
  applyMiddleware: [
    [
      session({
        path: "/",
        name: "app",
        maxAge: 2592000000, // 30 * 24 * 60 * 60 * 1000 expire after 30 days
        keys: [cookieSecret],
        httpOnly: true,
      }), 
      { routes: ['/api'] }
    ]
    [ morgan('tiny'),  { routes: [ '/api', '/_error' ]  } ],
    passport.initialize(),
    bodyParser.urlencoded({ extended: true }),
  ],
}

Or, if you wanted to include your own middleware function(s), then that is also supported (all routes middleware or conditional routes middleware):

module.exports = {
  applyMiddleware: [
    (req, res) => {
      res.setHeader('all-routes-middleware', 'hello world!!!!')
    },
    [
      (req, res) => {
        res.setHeader('conditional-routes-middleware', 'bye world!!!!')
      },
      { routes: ['/api/user'] },
    ],
  ],
}

Then in the Server class, these middlewares are applied to the IncomingMessage and ServerResponse parameters within the run method.

The advantage of this approach is that all of this abstracted from the developer, it's opt-in, doesn't require a _document page, and works in both server and serverless environments.

Let me know what you think.

@timneutkens
Copy link
Member Author

timneutkens commented Jan 23, 2020

@mattcarlotta that won't work as Server is not used in all cases when using Next.js, eg in serverless. Similarly next.config.js is only loaded at dev and build time and we're not going to add options that depend on bootup to next.config.js. We're increasingly discouraging adding the config options that require runtime like publicRuntimeConfig etc so that we can eventually not require next.config.js on next start as people tend to load tons of modules that aren't needed in production (to improve bootup time for next start). On serverless we already only load next.config.js on dev/build.

Also there's no clear correct behavior with introducing those methods, for example Next.js apps are increasingly hybrid of SSG / SSR and middlewares won't be able to run for static files as they'd be served from an edge cache.

@timneutkens
Copy link
Member Author

#7208 (comment)

This case is covered by the rewrites/redirects and new data fetching methods btw.

@nstfkc
Copy link

nstfkc commented Feb 7, 2020

Hey @timneutkens, I know this feature is still experimental but do you think is there any potential problem using this only to set some cookies. We only have SSR pages.

@timneutkens
Copy link
Member Author

If you want to set cookies it's better to use API routes and redirect.

@martpie
Copy link
Contributor

martpie commented Feb 13, 2020

Could this middleware be used in addition of app.render() or nextRequestHandler() @timneutkens ? Like in the custom server's examples https://github.com/zeit/next.js/blob/canary/examples/custom-server-typescript/server/index.ts

I am asking this because our stack includes a headless CMS (Cockpit) and Next.js, doing the routing between the two is currently (really) hard (the routes being on the CMS, and slugs/URLs/pages could be added/removed/edited any time by an editor).

In our scenario, the pages/ folder are more like a views/ or templates/ folder, and we need to be able to render any page based on the data we get from the CMS.

@sarapowers
Copy link

Hey all! We just launched an NPM library for Express style architecture in Next.js without adding an Express server. It might be helpful for your middleware challenges! Check it out if you're interested. https://github.com/oslabs-beta/connext-js

@runofthemill
Copy link

@sarapowers I'm about to check this out - what's the minimum version of Next it can be used with? Thanks for sharing!

@sarapowers
Copy link

@runofthemill At this point, Connext utilizes Next.js's API routes so 9.0 is the minimum version you'd need. We'd love to someday make it compatible with older versions though!

timneutkens added a commit to timneutkens/next.js that referenced this issue Apr 26, 2020
This option was experimental and doesn't cover the hybrid nature of Next.js (both static and dynamic pages). It also doesn't cover the cases we wanted to cover with the option. The option also does not work in all targets and is an avenue for slowing down the application.

Closes vercel#7208
@isaachinman
Copy link
Contributor

Hey @timneutkens - just want to close the discussion off here with some clarifying points. There are two main reasons why next-i18next has been driven to use a custom server in the past:

  1. Language detection itself, based on cookies/headers/etc
  2. Redirects, based on language

The second issue is helped along by rewrites - just waiting for that to land from experimental.

However, we still need to execute language detection middleware for each incoming frontend request (and potentially 302 in some cases). If this RFC is no longer being considered, I'm wondering if you have a suggestions as to the "best practice" way to do that?

I've set forward i18next/next-i18next#689 as an initial working idea of how to support serverless i18n with NextJs, and we're literally doing:

const middlewares = nextI18NextMiddleware(nextI18Next)
for (const middleware of middlewares) {
  await new Promise((resolve) => middleware(req, res, resolve))
}

Inside of a wrapped getInitialProps. Is this the user-land future of non-API middleware?

@timneutkens
Copy link
Member Author

timneutkens commented Apr 28, 2020

Current plan is to have the notion of languages / language detection built into Next.js, that way we can make it work with both SSG and SSR and have it work on the edge for the majority of cases. This will be similar to eg preview mode.

@isaachinman
Copy link
Contributor

Ah, I wasn't aware that was on the cards.

Perhaps naively, I don't see how that would solve, or even simplify, the problem of localisation in NextJs apps.

Language detection itself is a solved problem. The real trick with universal/NextJs apps is handling language detection in conjunction with saved state via cookies, and potential state via routing (locale subpaths).

There may be ways to get close to fully-static solutions by taking opinionated approaches to routing, but many valid use cases will always require middleware.

@omarryhan
Copy link

Current plan is to have the notion of languages / language detection built into Next.js, that way we can make it work with both SSG and SSR and have it work on the edge for the majority of cases. This will be similar to eg preview mode.

Can we get more info on that @timneutkens ? In what aspects of I18n will Next.js be involved and in what it won't?

@timneutkens
Copy link
Member Author

Can't share details yet. Still need to write up a RFC for it.

@fimbault
Copy link

fimbault commented Nov 27, 2020

Hi,

i18n support in nextjs10 is nice.
Beyond i18n, is there a more generic way of handling the original middleware requirement?
My use case is #19606

Thanks

@sebastian-nowak
Copy link

sebastian-nowak commented Feb 25, 2021

Is there any progress on this feature? There really should be a way to add a middleware for all pages, not just APIs, that can set headers/cookies globally without disabling automatic static optimization.

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.