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

[feat] expose handler to allow use in custom server #2051

Conversation

matths
Copy link
Contributor

@matths matths commented Jul 31, 2021

I stumbled across a problem similar to the ones in #334.

Currently the build process using adapter-node exports a Polka instance listening to a configurable host/port.
This way the SvelteKit app is the only requestHandler, so to say.

So my proposal is to export the app as a requestHandler instead.
A requestHander ((req, res) => {}) is very similar to an express like middleware ((req, res, next) => {}), just missing the next()callback.
This way you could combine the SvelteKit app with other requestHandlers and/or middleware.

Example
Provided using adapter-node from this branch, you could have the option asRequestHandler in your svelte.config.js

kit: {
	adapter: adapter({
		out: 'build',
		asRequestHandler: true,
		env: {
		}
	}),
	target: '#svelte'
}

When calling npm run-script build in your project, you can no longer run your build using node build.
Instead you need to implement your own server (or express application) and import the build instead.

import http from 'http';
import { requestHandler } from './build/index.js';

const server = http.createServer(requestHandler);
server.listen(5000, '0.0.0.0');

I haven't tested with express yet, but it should look similar to this

import { requestHandler } from './build/index.js';
import express from 'express';

const app = express();

var myLogger = function (req, res, next) {
  console.log('LOGGED');
  next();
};

app.use(myLogger);

app.get('/no-svelte', (req, res) => {
  res.send('Hello World!')
});

app.get('*', requestHandler); // SvelteKit

app.listen(5000)

Tests
As the added code is mostly in files that are object of the build process, I haven't found a good example on how to come up with a test. I did some manual testing of the functionality of course.

Changesets
no changeset provided, yer.

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2021

⚠️ No Changeset found

Latest commit: 4dcde29

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Rich-Harris
Copy link
Member

What if instead of asRequestHandler, the adapter outputted two files — a server.js (similar to the current index.js) but also an index.js that exposed the bits you need, and which server.js imported from?

// index.js
export const paths = {
  assets: join(__dirname, '/assets'),
  prerendered: join(__dirname, '/prerendered')
};

export const handler = (req, res, next) => {...};

That way you could either run the 'reference implementation' server-in-a-box — node build/server — or you could import the handler and do whatever you want with it. We'd need to figure out the details, since the exports from that file would become API rather than implementation detail, but does that feel sufficiently flexible/future-proof? A thing about this approach that's potentially quite nice is that we can add new stuff (e.g. we could later decide to expose the sirv instances) without making the choice of name of asRequestHandler (or whatever else we might come up with) becoming outdated as needs evolve.

@matths
Copy link
Contributor Author

matths commented Aug 1, 2021

Yes, that sounds legit and possible.
You're talking about two targets in rollup.config.js from the adapter-node package?

So also the 'reference implementation' would then use the fake server object to trick listen() from polka, just to make a plain node httpServer on its own.
https://github.com/lukeed/polka/blob/2ce10df97921126ffcc43e23aa14af18f93f49e1/packages/polka/index.js#L65

(I forgot to mention, that having your own node and/or express server using this approach would make #2048 obsolete as this could be achieved in 'userland'.)

@benbender
Copy link

I like this approach very much as this would render most of the open issues asking for middleware's, server-side-events, websockets etc obsolete, as all of this could be easily achieved in userland with your custom server. All of that without compromising on sveltekit's (serverless) core and while giving enough freedom for future technologies. \o/ 💯

@jsprog
Copy link

jsprog commented Aug 3, 2021

@benbender, I really like this approach hoping that it'll land very soon. At the same time, we need to understand the needs of many developers to receive the same experience when coding (dev) without resorting to other complexities (like proxies), complex and non stable hacks.

Please, see my comment here: #887 (comment)

@matths
Copy link
Contributor Author

matths commented Aug 4, 2021

I am able to split inside packages/adapter-node (files/ directory) into the two portions mentioned by @Rich-Harris (an index.js and a server.js), but I am struggeling what I need to change in the the later step, when running node build/server in userland project is called.
So I am not yet pushing my recent commits as they would break the status quo.
Some help would be appreciated. Feel free to contact me and/or have a chat/teams call.

@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
@benmccann
Copy link
Member

I think I'd override the esbuild entrypoint to point at your own file instead of the default: https://github.com/sveltejs/kit/tree/master/packages/adapter-node#esbuild

Your own file could look pretty similar to the default, but would need a different file path to the app:

import { init, render } from '../.svelte-kit/output/server/app.js'; // assuming this file lives in src this would be the path to app.js
import { host, port } from './env.js'; // eslint-disable-line import/no-unresolved
import { createServer } from './server';

init();

const instance = createServer({ render }).listen(port, host, () => {
	console.log(`Listening on ${host}:${port}`);
});

export { instance };

@benmccann
Copy link
Member

Btw, I'm on the Svelte Discord under this username if you want to ping me with any questions

@matths
Copy link
Contributor Author

matths commented Aug 5, 2021

@benmccann I've managed to come up with my interpretation of @Rich-Harris comments somehow. Feel free to have a look and give feedback. I had some struggle with externalising the handler from the reference server. I also did not expose anything other than the handler/middleware itself from the whole polka/sirv world, yet, so no new API. I hope I didn't break existing API though.
It's working perfectly for my use case. ;)

packages/adapter-node/README.md Outdated Show resolved Hide resolved
packages/adapter-node/README.md Outdated Show resolved Hide resolved

You can of course your favorite server framework to combine with other and/or custom middleware, e.g. [express](https://github.com/expressjs/expressjs.com).

```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other questions the docs should answer: Where do you put this file? Is there some option you need to specify to make it use this vs the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean. Using it this way is kind of advanced stuff for experienced node users. So I am not sure how to verbose to document. I'm not a native speaker, so feel free to come up with better documentations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if you want to create your own server using the code below does it matter what the file path or name is?

packages/adapter-node/index.js Outdated Show resolved Hide resolved
@benmccann

This comment has been minimized.

@matths

This comment has been minimized.

@benmccann

This comment has been minimized.

@benbender

This comment has been minimized.

@benmccann

This comment has been minimized.

@benmccann benmccann marked this pull request as ready for review August 20, 2021 05:07
@benmccann benmccann changed the title Let polka from adapter-node be just another requestHandler [feat] expose handler to allow use in custom server Aug 20, 2021
@benmccann benmccann force-pushed the feature/adapter-node-inject-existing-server-in-polka branch 3 times, most recently from 1b10a92 to 8d7c377 Compare August 20, 2021 05:25
@benmccann benmccann force-pushed the feature/adapter-node-inject-existing-server-in-polka branch 2 times, most recently from aeb0a82 to 0ea47e7 Compare August 25, 2021 16:46
@benmccann
Copy link
Member

@matths I've rebased this a couple more times to keep it up-to-date with master for you. Do you think you'll be able to take a look at my comments? I'd love to get this change in

@pranaypratyush
Copy link

Very eagerly waiting for this feature.
I am getting the following error. Is this expected or because I am using gitpkg?

> Using @sveltejs/adapter-node
 > error: Could not read from file: /home/pranay/wd/sveltekit-ws/node_modules/.pnpm/@gitpkg.now.sh+matths+kit+packages+adapter-node?feature@adapter-node-inject-existing-server-in-polka/node_modules/@sveltejs/adapter-node/files/shims.js

 > error: Could not resolve ".svelte-kit/node/handler.js"

> Build failed with 2 errors:
error: Could not read from file: /home/pranay/wd/sveltekit-ws/node_modules/.pnpm/@gitpkg.now.sh+matths+kit+packages+adapter-node?feature@adapter-node-inject-existing-server-in-polka/node_modules/@sveltejs/adapter-node/files/shims.js
error: Could not resolve ".svelte-kit/node/handler.js"
Error: Build failed with 2 errors:
error: Could not read from file: /home/pranay/wd/sveltekit-ws/node_modules/.pnpm/@gitpkg.now.sh+matths+kit+packages+adapter-node?feature@adapter-node-inject-existing-server-in-polka/node_modules/@sveltejs/adapter-node/files/shims.js
error: Could not resolve ".svelte-kit/node/handler.js"
    at failureErrorWithLog (/home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:1449:15)
    at /home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:1131:28
    at runOnEndCallbacks (/home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:1049:65)
    at buildResponseToResult (/home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:1129:7)
    at /home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:1236:14
    at /home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:609:9
    at handleIncomingPacket (/home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:706:9)
    at Socket.readFromStdout (/home/pranay/wd/sveltekit-ws/node_modules/.pnpm/esbuild@0.12.22/node_modules/esbuild/lib/main.js:576:7)
    at Socket.emit (node:events:394:28)
    at Socket.emit (node:domain:475:12)
 ERROR  Command failed with exit code 1.

@matths
Copy link
Contributor Author

matths commented Aug 26, 2021

Very eagerly waiting for this feature.
I am getting the following error. Is this expected or because I am using gitpkg?

@pranaypratyush, maybe. I do not know gitpkg.

For my tests I have sveltekit checked out locally, build the node-adapter with pnpm build and copied the build in a subfolder of my sveltekit project. Inside my projects svelte.config.jsI do import that node-adapter instead the one from the node_modules folder.

// import adapter from '@sveltejs/adapter-node';
import adapter from './src/lib/adapter-node-request-handler/index.js';

@JeanJPNM
Copy link
Contributor

JeanJPNM commented Aug 26, 2021

copied the build in a subfolder of my sveltekit project.

Try using pnpm link for that, it also works for pretty much any package that you use it on.

@benmccann benmccann force-pushed the feature/adapter-node-inject-existing-server-in-polka branch 7 times, most recently from f58942c to 26d9f1b Compare August 27, 2021 04:42
@benmccann benmccann force-pushed the feature/adapter-node-inject-existing-server-in-polka branch from 26d9f1b to 4dcde29 Compare August 27, 2021 04:45
@benmccann benmccann merged commit 8f67e46 into sveltejs:master Aug 27, 2021
@mirabledictu
Copy link

If I understand correctly, we still need to build the sveltekit app before we can use custom server? (e.g., express, polka)

What's the difference then between this or just having an external server? :/

@matths
Copy link
Contributor Author

matths commented Aug 27, 2021

@mirabledictu you now don't need two node servers, one for your other middleware and one to serve your sveltekit app.
So it's more of an integration of your sveltekit app into an existing node server application than otherway around.
But of course this way, you could have your server serve other endpoints beside the ones from sveltekit or even implement websockets and the like.

But please be fully aware, that @Rich-Harris prefers to hold off an escape hatch like this in favor of being portable and support serverless and other adapters beside plain node.

The goal is to make SvelteKit apps as portable as possible, which definitely doesn't preclude adding non-portable things like direct access to underlying primitives like res eventually. I do think it's worth holding off as long as possible so that we're able to clearly articulate the problems we're trying to solve and see how well we can solve them in a portable way.

Originally posted by @Rich-Harris in #1563 (comment)

So this PR is an escape hatch specifically for running sveltekit as one-of-many middlewares in another node, express, polka or whatever node app server. It's absolute valid to help the others to come up with a more sveltish way to solve the underlying problems / use cases. ;)

@alastaircoote
Copy link

Thanks for this, I've just been playing around with it and it works great. One issue, though: the client-side router isn't aware of any routes created via a custom Express server (obviously), so it shows a 404. Any thoughts on an approach to take here?

I tried creating an __error.svelte file that calls window.location.reload() on the client when it encounters a 404 to force a server load but's pretty gross (and still shows a flash of the error component). I'm looking through the docs to find something more graceful but so far nothing. I wonder if there could be an option in the client-side router to always defer to a server load when it doesn't match the route, something like that?

@benmccann
Copy link
Member

What's the difference then between this or just having an external server?

This PR will let you add middleware in front of SvelteKit, which you cannot do with an external server. Also, the question of what's the difference between having one server or two servers seems like an obviously silly one. It's less processes to manage, you have a single build process, everything is in a single codebase without a need for shared libraries, etc. I'm not really sure what else you were looking for?

the client-side router isn't aware of any routes created via a custom Express server (obviously), so it shows a 404

You can set rel=external. Does that solve your issue?
https://kit.svelte.dev/docs#anchor-options-rel-external

@evdama
Copy link

evdama commented Feb 16, 2022

@matths and @benmccann, thanks a lot for your work on this subject 👍

https://discord.com/channels/457912077277855764/943462063261511680 discibes in detail the question how you could pass information from an express session onto sveltekit when you use src/server.js which imports handler() from build/handler.js like so

// src/server.js - here I use express middleware to read a users IP address before I hand over to svk using handler()

import { handler } from '../build/handler.js'
import express from 'express'
import session from 'express-session'
import parser from 'ua-parser-js'

const app = express()

if (user-from-outside-Europe) {
// a route that lives separately from the SvelteKit app
app.get('/non-svk-route', (request, response) => { response.end('non-svk route...') })
}

// let SvelteKit handle everything else, including serving prerendered pages and static assets
app.use(handler)
app.listen(3000, () => { console.log('hey...listening on port 3000') })

So how would one pass data from an express session onto sveltekit so it could be used with event.locals inside hooks.js for example?

@benmccann
Copy link
Member

You can't right now, but it would be a very simple change to SvelteKit to enable it. There's a platform field that can be populated by the request (e.g. adapter-cloudflare uses it). However, the team would like to see a usecase that can't be solved any other way before adding this in adapter-node. Based on another thread in Discord it sounded like your usecase could probably be solved by handle, but if that's incorrect please file a feature request explaining in detail what you're trying to accomplish and why it can't be done with the existing APIs

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

Successfully merging this pull request may close these issues.

Make SvelteKit compatible with server middlewares
10 participants