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

Expose a way to inject middleware in the server pipeline for adapt-node #334

Closed
samccone opened this issue Jan 20, 2021 · 17 comments
Closed
Labels
p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Milestone

Comments

@samccone
Copy link
Contributor

Similar to #333

It would be valuable to be able to insert additional handlers in the adapt-node server pipeline to customize the serving behavior based on deployment env.
https://github.com/sveltejs/kit/blob/master/packages/adapter-node/src/server.js#L30

In sapper we directly exposed the server making this possible but now in sveltejs + adapt-node we do not expose this dial. It would be nice to have an escape hatch

@Rich-Harris
Copy link
Member

I feel like we need to be specific about the use cases here. The downside of a catch-all escape hatch is that it means that the answer to various problems becomes 'use adapter-node, because you can use arbitrary middleware' which is the opposite of what we want out of a serverless framework. I think it would be better to be able to satisfy those uses across different adapters, but it's hard to know what the relevant APIs look like unless we have a clear handle on what those uses are

@benmccann
Copy link
Member

People are going to have use cases we haven't imagined. I'm not sure we should block them from being able to use SvelteKit. I really think this is something that's going to be needed

Just a few minutes ago someone was complaining on Discord that they can't use passport. I've already been frustrated by not being able to use Multer or Formidable (#70 (comment)). There may be solutions to these that we can build that would be better, but there's a whole ecosystem of solutions out there that we're stopping people from using and it seems impossible that we'd reimplement all of them

@Rich-Harris
Copy link
Member

File uploads should be handled out of the box, and serverless apps need auth too. I really don't think multer, formidable and passport are the right answers in 2021

@dummdidumm
Copy link
Member

I think those were just some specific examples, showing that in general it's not really predictable what the whole JS ecosystem does, and that it may need support for middleware from SvelteKit. Also I don't think we should prevent people from using libraries which are battle-tested and which they want to use. If this was supported and people would reach more for adapter-node, I don't think this would be a problem. It's the user's decision to do this after all. It also would help us understand the use cases better and then maybe provide a solution which works in serverless environments, too, later on.

@GrygrFlzr
Copy link
Member

I'm crazy, so the way I've been doing this for a production app is:

  • Dev mode runs an Express + GraphQL + Passport server which proxies requests to SvelteKit's dev server with Axios
  • Prod mode imports the optimized build from svelte-kit build (with a blank adapter) into the express server and uses it as a request -> response black box

In lieu of an actual way to do "serverfull" projects, this kind of hacky thing will sprout up.

@Rich-Harris
Copy link
Member

And that's fine — that's the system working. Having a little bit of friction gives you a much clearer picture of which use cases warrant first-class support. If the frictionless escape hatch is so big that everything just gets dumped in there, it means the framework isn't providing sufficient value, especially if it steers people away from serverless platforms. (Short of @GrygrFlzr's solution, adding arbitrary middleware to adapter-node doesn't even really solve the problem, because it doesn't apply to svelte-kit dev and svelte-kit start.)

If the question is 'what do you need to do?' the answer is never 'I need to use Multer' or 'I need to use Formidable' or 'I need to use Passport'; it's 'I need to handle file uploads' or 'I need to use OAuth providers for authentication'. These are solvable problems, and I would go so far as to argue that those two problems fall into SvelteKit's domain.

The future-proof thing to do isn't to throw our hands up and say 'we can't possibly anticipate everything people will need to do, therefore we should abdicate the responsibility to the ecosystem of Express middleware', it's to understand (from specific examples) how to make it possible for people to solve arbitrary problems in an environment-agnostic way (i.e. doesn't rely on req and res). Express isn't going anywhere, but it's also not the future of anything. The future is serverless and Cloudflare Workers and Netlify Edge Handlers (and Deno?).

To be clear, I'm not suggesting that SvelteKit should include equivalents for every Express or Fastify middleware out there, just that we should provide the necessary hooks for an ecosystem of such things to exist.

@benmccann
Copy link
Member

I think the goal of adapters should be to be quite flexible. If people want to user serverless that's great and if they don't that's also great. Serverless may not be the best for every use case and we shouldn't force it on people.

I agree we should support all these use cases in serverless environments. Passport offers authentication with over 500 different methods (oauth 1, oauth 2, bearer tokens, OpenId, freeagent, TOTP, active directory, etc., etc.). It seems like a terrible burden to place on the community that they should have to reimplement passport and all its plugins in a SvelteKit-compatible way using our unique APIs if you're a user for whom adapter-node is legitimately the best solution and there's already a middleware available that would solve the problem. If you have a usecase where running a Node.js server is preferable to running in serverless then I don't think we should stop you from using the entire ecosystem of already developed solutions. There are other ways we can find out the biggest needs of the community. We'll still get reports in the issue tracker, can look at the most popular plugins in other frameworks and servers, etc.

@Conduitry
Copy link
Member

I was recently in the position of wanting a Sapper app with a really quick and dirty basic auth thing in front of it. (It wasn't anything highly sensitive, I was just trying to prevent randos from using up the API call allotment for an underlying service.) I didn't want a fully fledged proper authentication system, I just wanted to be able to insert some code that checked for a specific HTTP header and either let the request proceed or intervene and return a 401. This was trivial to do when I was able to write Express middleware.

It's not a perfect API, but the Express middleware contract is both well known and lets people do just about whatever weird shit they'd like. There are going to be tons of problems that are way too niche to have an official tailored solution for. (And I wouldn't say that just because people are having trouble listing them doesn't mean that they don't exist.) Coming up with another API that was as flexible sounds like a hassle for us because we'd need to outsmart this very battle-tested API, and also sounds like a hassle for users, because it's something new they need to learn.

@Rich-Harris
Copy link
Member

I'll be (perhaps too) honest: Every time I use Passport I weep. I'm always copying and pasting a bunch of code that I don't really understand, and I have absolutely no idea what it's doing under the hood. I daresay that '500 different methods' overestimates the scale of the problem; there's a very long tail.

I get that there's a rich ecosystem of Express middleware. There's also a rich ecosystem of React components! You can use those React components in your SvelteKit app if you really want to, but there's going to be friction, because we don't believe that that's the best way to build apps.

So I think we need to look at this the same way: it should probably be possible to use Passport and other middleware, but I don't think we should encourage it by making it idiomatic. Making everything except adapter-node a second-class citizen feels a bit 'this site works best in Internet Explorer' to me.

@Conduitry
Copy link
Member

What would you consider a non-idiomatic way of using other middleware? Using a forked version of adapter-node that supports that?

@Rich-Harris
Copy link
Member

If adapters had some way to intervene in svelte-kit dev and svelte-kit start then yeah, a fork has probably the right amount of friction

@Rich-Harris
Copy link
Member

To recap conversation here and on Discord:

  • it's not enough to have a way to add middleware to adapter-node, because it doesn't apply to svelte-kit dev and svelte-kit start
  • ideally there should be some way to use existing connect middlewares, but we don't want to privilege adapter-node over serverless adapters (or CFW etc, when we get round to it)

I propose that we add a handler function that serves as a catch-all request handler, making it possible to deal with use cases like #334 (comment)...

export function handler(request, render) {
  if (!request.headers['x-the-header-conduitry-was-using']) {
    return { status: 401 };
  }

  // otherwise fall through to default handler
  return render(request);
}

...but also to support existing middlewares in an environment-agnostic way, albeit in a way that makes people feel just nauseous enough to consider a more idiomatic solution:

import { applyMiddleware } from 'some-third-party-library';

const middleware = get_auth_middleware_somehow({...});

export function handler(request, render) {
  return applyMiddleware(request, render, middleware);
}

In this example, applyMiddleware works something like this:

// this is incorrect and incomplete, purely for illustration
function applyMiddleware(request, render, middleware) {
  return new Promise((fulfil, reject) => {
    const req = {
      url: request.path,
      method: request.method,
      headers: request.headers,
      // ...
    };

    const res = {
      status: null,
      headers: {},
      body: '',
      writeHead(status, headers) {
        res.status = status;
        Object.assign(res.headers, headers);
      },
      write(data) {
        res.body += data;
      },
      end() {
        fulfil({
          status: res.status,
          headers: res.headers,
          body: res.body
        });
      }
    };
  });

  middleware(req, res, err => {
    if (err) {
      reject(err);
    } else {
      fulfil(render(request));
    }
  });
}

There isn't a great place to put this — setup doesn't feel like the right word. But maybe if we renamed setup to hooks, it could live there alongside prepare and getSession:

// src/hooks.js
export function prepare({ headers }) {
  return {
    context: {...},
    headers: {...}
  };
}

export function getSession({ context }) {
  return {...};
}

export function handle(request, render) {
  return render(request);
}

Arguably, returning headers from prepare is unnecessary, and it should be like this instead:

// src/hooks.js
export function getContext({ headers }) {
  const context = {...};
  return context;
}

export function getSession({ context }) {
  return {...};
}

export function handle(request, render) {
  const rendered = render(request);

  return {
    ...rendered,
    headers: {
      ...rendered.headers,
      'x-custom-header': 'blah'
    }
  };
}

(I also think that maybe context belongs on the request object, as request.context, as it's easy to pass around that way.)

Thoughts?

@pixelmund
Copy link
Contributor

pixelmund commented Mar 25, 2021

@Rich-Harris

I started working on this, primarily to learn the Kit Codebase better. Don't know if its the right approach but it works in the sandbox.

You can take a look here, i probably won't open a pr because i don't have enough time to continue: https://github.com/pixelmund/kit/tree/add-handle-and-hooks

Rich-Harris added a commit that referenced this issue Mar 25, 2021
Rich-Harris pushed a commit that referenced this issue Mar 25, 2021
* replace setup with hooks (#334)

* changeset

* lint

* docs and types

* rename empty setup module

* tweak wording

* fix realworld example

* lets just make it async

* explain resolve_entry

* reduce indentation
@Rich-Harris
Copy link
Member

Closing this now that we have hooks: https://kit.svelte.dev/docs#hooks

@babeard
Copy link

babeard commented Apr 2, 2021

People are going to have use cases we haven't imagined.

Server-sent events may be such a use case. It doesn't seem possible with hooks. applyMiddleware seems like a good fit for this.

@cayter
Copy link

cayter commented Jul 10, 2021

I'm crazy, so the way I've been doing this for a production app is:

  • Dev mode runs an Express + GraphQL + Passport server which proxies requests to SvelteKit's dev server with Axios
  • Prod mode imports the optimized build from svelte-kit build (with a blank adapter) into the express server and uses it as a request -> response black box

In lieu of an actual way to do "serverfull" projects, this kind of hacky thing will sprout up.

@GrygrFlzr I came up with something exactly like what you mentioned here.

@matths
Copy link
Contributor

matths commented Jul 31, 2021

How about making it easier to combine SvelteKit adapter-node generated code with other connect/express like middleware by not exporting a full featured polka server instance, but a requestHandler instead?
I made a draft here: #2051

@benmccann benmccann added this to the 1.0 milestone Aug 4, 2021
@benmccann benmccann added the p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. label Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Projects
None yet
Development

No branches or pull requests

10 participants