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

Open
timneutkens opened this issue May 1, 2019 · 32 comments
Open

[RFC] server(less) middleware #7208

timneutkens opened this issue May 1, 2019 · 32 comments
Labels
RFC

Comments

@timneutkens
Copy link
Member

@timneutkens 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

This comment has been minimized.

Copy link
Member Author

@timneutkens timneutkens commented May 1, 2019

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

This comment has been minimized.

Copy link

@ivan-kleshnin 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

This comment has been minimized.

Copy link
Contributor

@isaachinman isaachinman commented May 2, 2019

@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

This comment has been minimized.

Copy link
Member Author

@timneutkens timneutkens commented May 2, 2019

@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

This comment has been minimized.

Copy link
Contributor

@Janpot 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

This comment has been minimized.

Copy link
Member Author

@timneutkens timneutkens commented May 2, 2019

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

This comment has been minimized.

Copy link
Contributor

@isaachinman isaachinman commented May 2, 2019

@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

This comment has been minimized.

Copy link
Contributor

@jesstelford jesstelford commented May 10, 2019

♥️ 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

This comment has been minimized.

Copy link
Contributor

@isaachinman isaachinman commented Jul 9, 2019

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 (isaachinman/next-i18next#274).

@trevordmiller

This comment has been minimized.

Copy link
Contributor

@trevordmiller 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

This comment has been minimized.

Copy link

@zomars 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

This comment has been minimized.

Copy link
Contributor

@isaachinman isaachinman commented Sep 8, 2019

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

@amardeep9911

This comment has been minimized.

Copy link

@amardeep9911 amardeep9911 commented Sep 8, 2019

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

@hoangvvo

This comment has been minimized.

Copy link

@hoangvvo 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

@HelloEdit

This comment has been minimized.

Copy link

@HelloEdit HelloEdit 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

This comment has been minimized.

Copy link

@game7 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

@HelloEdit

This comment has been minimized.

Copy link

@HelloEdit HelloEdit 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

This comment has been minimized.

Copy link

@callmenick callmenick commented Oct 16, 2019

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

@timneutkens

This comment has been minimized.

Copy link
Member Author

@timneutkens timneutkens commented Oct 16, 2019

It wouldn't work with static pages.

@hoangvvo

This comment has been minimized.

Copy link

@hoangvvo hoangvvo commented Oct 21, 2019

Hey @timneutkens, is the middleware also applied to API Routes or just non-api pages? Thanks!

@sandys

This comment has been minimized.

Copy link

@sandys sandys commented Oct 21, 2019

hi guys,
I would like to chime in with the need for combining this with something that was referred in #9013

I'm personally of the opinion that nextjs should adopt expressjs and its middleware and not end up rebuilding a lot of stuff.
For example, we also use expressjs middleware to give seamless msgpack based responses (https://github.com/textbook/express-msgpack). A bunch of people do this for protocol buffers, etc as well.

there are tons of these kind of examples and i fear that nextjs will go through a long cycle rebuilding all of them (or expect end users to reimplement them)

@hoangvvo

This comment has been minimized.

Copy link

@hoangvvo hoangvvo commented Oct 21, 2019

@sandys In my opinion, adding too many things will make Next.js bloated, potentially hurting its performance and optimization. Also, not everyone's configuration is the same, so having several defaults may break things of those who don't use them. It is still safer to implement personally.

@sandys

This comment has been minimized.

Copy link

@sandys sandys commented Oct 21, 2019

@martpie

This comment has been minimized.

Copy link
Contributor

@martpie martpie commented Oct 21, 2019

@sandys I think you're missing the point.

Zeit.co is betting on serverless and one of the core feature of Next.js is serverless support. Express completely defeats this philosophy/architecture, as you need a centralised entrypoint/routing.

So you can be sure Express.js won't be used as a base (and it's why Next uses micro)

@timneutkens

This comment has been minimized.

Copy link
Member Author

@timneutkens timneutkens commented Oct 21, 2019

Using express is not needed for Next.js. But on the same note micro is not used for Next.js either.

Depending on the output target used the Node.js plain HTTP server is used which ensures the best performance (no other overhead). In the serverless target we just provide the req/res API as a low-level API.

Furthermore bundling express would mean the bundle size for serverless functions would be significantly larger.

@goldenshun

This comment has been minimized.

Copy link
Contributor

@goldenshun goldenshun commented Oct 23, 2019

At one point I was thinking these middlewares might also execute on the client but after reading this again, I am thinking the plan is to only execute these on the server. Is that a correct assumption?

@sandys

This comment has been minimized.

Copy link

@sandys sandys commented Oct 23, 2019

@timneutkens thanks for replying - your answer is what i always assumed. At the end of the day, that is precisely the tradeoff you have to decide.

expressjs is 300-400kb (https://bundlephobia.com/result?p=express@4.17.1) . For those of us who dont do serverless, it unlocks a vast ecosystem of readymade middleware.

A middle ground (!!) is to maintain the expressjs API, but implement it ourselves. That way, Nextjs can provide a minimal size for serverless, while being able to plug into a large ecosystem for others.

@timneutkens

This comment has been minimized.

Copy link
Member Author

@timneutkens timneutkens commented Oct 23, 2019

@goldenshun correct. Note that at this point in time (few months are we release Next.js 9) I don't strongly believe this RFC is the way to go anymore. But I'm still thinking about it. Especially with plugins we could have a next-plugin-helmet (for security headers) and the other cases for middleware could be covered too.

@sandys I don't fully understand what you're trying to achieve on this thread. This RFC covers an edge case where people want to get rid of their custom server.

You can already run express if you want to as per the custom server API: https://github.com/zeit/next.js#custom-server-and-routing

Overall with Next.js 9 it's much less likely you'd need a custom server anyway.

@benhjames

This comment has been minimized.

Copy link
Contributor

@benhjames benhjames commented Oct 23, 2019

I think there's still a good argument for middleware support that the current plugin RFC doesn't cover.

For example, something like next-plugin-helmet would need an additional plugin hook that can access and modify the Request and Response objects to use and set headers.

// on-request.js
const handler = helmet(config);

export default async function onRequest({ req, res }: { req: Request, res: Response }): void {
  // called upon receiving a request, allows you to modify response headers, e.g.:
  handler(req, res);
}

This proposed hook sounds like a good point for discussion generally, however, using environment variables as config isn't ideal, because helmet config can be complicated (nested objects) and use dynamic values based on the Request object:

const config = {
  contentSecurityPolicy: {
    scriptSrc: ["example.com", (req) => req.locals.nonce]
  }
}

This couldn't be configured with the nextjs key in the package.json of the plugin so would need an additional way of loading config for it to be a plugin.

Keen to hear thoughts 👍

@goldenshun

This comment has been minimized.

Copy link
Contributor

@goldenshun goldenshun commented Oct 23, 2019

@timneutkens I believe you are correct that the plugins system would cover many cases that could have been covered by a server-only middleware.

As I mentioned in my comment on the Custom Routes RFC, I am still on the hunt for a better solution to dynamic redirect logic than getInitialProps and hoped page level middleware that executes on both client and server could be a solution. The part that confuses people with redirects within getInitialProps is that there is no way to halt the page from rendering. We always need to have custom code that looks like this:

const Page = ({ isUserAuthenticated }) => {
  if (!isUserAuthenticated) return null; // bail since Page is going to render even when we call redirectToLogin.

  return (<div>...</div>);
}
Page.getInitialProps = async (ctx) => {
    if (!userIsAuthenticated(ctx) {
      logger.info('accessToken not found, redirecting user to login');
      redirectToLogin({ redirectTo: ctx.asPath, ctx });
      return {};
    }

    return { isUserAuthenticated: true };
  };

So I think I was looking for something similar to the original proposal in this thread:

export function middleware(ctx, next) {
  const { req, res } = ctx; // Same ctx that will eventually be passed to getInitialProps
  if(shouldRedirect(...)) {
    if (res) {
      res.redirect('/login');
    } else {
      Router.push('/login'); // also works on the client
    }
  }
  next(); // Similar to express, call the next middleware in the chain but only if we aren't redirecting
}

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

export default IndexPage

This is actually the only use case I can think of that would take advantage of this style of middleware, so we might be able to narrow the scope of the problem by making this an explicit redirect function:

export function redirect(ctx) {
  if(shouldRedirect(ctx)) {
    return '/login';
  }
  return false; // null, undefined, etc. means no redirect
}
@ibraelillo

This comment has been minimized.

Copy link

@ibraelillo ibraelillo commented Nov 20, 2019

I personally think this makes a lot of sense since it will allow us to throw a whole bunch of external middlewares for express/koa/any other server environments. Some kind of precious uniformity with next ecosystem can be seen at the end of the tunnel :)

I strongly think this can leverage the case of having hard/heavy implementations on plugins and middlewares coming from all sort of ecosystems.

We use i18n and we're forced to do custom server, or serve the translations trough some lamdas from pages/api. It seems over engineered for me.

Cookies parsing, body parsing and some other middlewares already presents for api routes, could eventually be used here (opt-in, of course).

Some other practical cases are : check authentication tokens for every request, ab-testing redirections, etc.

I really hope this RFC comes out and help us all.

And thanks to all the zeit team for this amazing work, you're doing great, guys.

@BjoernRave

This comment has been minimized.

Copy link

@BjoernRave BjoernRave commented Nov 28, 2019

so as far as i understand the plugins rfc is kind of the v2 of this. alot of people are struggling with serverless i18n, @timneutkens is there anything you can say/share about this, maybe especially regarding support of next-i18next? isaachinman/next-i18next#274

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.