Preserve the original request when using adapter-node

[chrskerr]

The purpose of this PR is to retain access to the original request from Express during SvelteKit SSR.

We run session and auth middleware on all Express requests before we invoke the SvelteKit adapter-node handler. These set keys (sessionID and user) on the request object which can then be used within other handlers to assess auth status.

This PR will allow us to access these from the originalReq object and extend our auth mechanism into sveltekit.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: d671805

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/adapter-node	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

@chrskerr is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

[Conduitry]

This has come up before - I don't remember whether there's still an open issue about it. But this API isn't how we would want to do it anyway - we shouldn't be throwing additional values onto the request object. event.platform exists for this purpose, if we do want to expose this information.

[chrskerr]

Thanks @Conduitry! I can't see where I would access event.platform within adapter-node, do you have any pointers please of the best place for me to start?

I have pushed again with the keys inserted into platform rather than the request. There will now be naming and other implications which I haven't completed yet, but if you are happy with this general approach then I will finish it off :)

Thanks!

[dummdidumm]

Is there any drawback to pass on the whole req object to platform? That would remove the need for the config option.
server.response(request, { req }, ..)

[chrskerr]

No concern from my end, I had just gone the other way on a guess that more specificity would be preferred.

Naming wise, I was thinking originalReq or originalRequest?

[chrskerr]

@dummdidumm @Conduitry sorry to chase, just keen to know what I can do to get this mergable please 🙂 it will be a huge help for our app

[dummdidumm]

Code-wise this looks good, only open point to me is the name (who would have thought 😄 ). req is kind of the "standard" name in the node world, so I'm inclined to call it just req, because it's already on the platform object the original prefix could be superfluous. Not saying you should change it right away, open to discussing the name and maybe keeping it as is (also want other maintainers to chime in).

[chrskerr]

I had leant towards originalReq to help disambiguate between the original express request and the sveltekit request which is also available to users. This clarity is nice since originalReq will not be available during dev, and it reduces the chances of using the wrong request by mistake and having unpredictable results.

Having said that I'm personally okay with both naming choices, the simplicity of req is much more standard and clean than originalReq 😃

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
kit	🔄 Building (Inspect)		Jan 5, 2023 at 4:48PM (UTC)

[Rich-Harris]

Moved the declaration to ambient.d.ts because the declare needs to be in a module that doesn't export anything if TypeScript is to recognise it. With that change (and updating the type from Request to http.IncomingMessage) I get this autocompletion, which is nice:

Though in order to get types for the custom properties, you'd need to do something like this:

declare namespace App {
  export interface Platform {
    req: import('http').IncomingMessage & {
      sessionID: string;
      user: {...}
    };
  }
}

I still think event.platform.req is probably a bit of a footgun — basically the only time you'd ever need to access the object is if you're using middleware that should probably be reimplemented as something that works with Request and Response natively, so that it can work with dev and preview. The fact that so much Express middleware monkey-patches req is a symptom of a flawed design, and I feel uncomfortable about blessing that pattern rather than helping to kill it off once and for all, especially since SvelteKit apps built today are by definition greenfield.

That said, pragmatism may require us to look the other way, so if other maintainers are in favour of this change then I won't kick up a stink.

[Rich-Harris]

Consensus was that we should do this. Thanks!