[feat] adapter module

[ignatiusmb]

Resolves #2249
Fixes Too much for #625, better as a separate PR

Exposes two new modules, @sveltejs/kit/adapter and @sveltejs/kit/app. Instead of importing from ../output/server/app.js, adapter authors can now do this instead

import app from '@sveltejs/kit/app'; // can also be named import, { init, render }

app.init()

// somewhere, app.render()

[changeset-bot]

⚠️ No Changeset found

Latest commit: 15f6149

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

[benmccann]

The only thing I wonder about the async / await thing is that top-level await isn't available until Node 14.8, so maybe the adapters wouldn't work with Node 12. But this change does look really nice

[ignatiusmb]

This is indeed too good to be true. I'm not sure if it's just the node adapter or others as well, but module resolutions and bundling don't play well with each other nicely. Would need to go back to the drawing board again for some time and dig deeper into the bundler options before continuing this. Type checking should still be fixed with this if all else fails.

[JeanJPNM]

Maybe we could use @rollup/plugin-alias to replace @sveltejs/kit/adapter by ../output/server/app.js

[jthegedus]

Raising the min version to Node 14.8 isn't the worst thing to happen. Node 12 is only considered in Maintenance until April 2022. Hell, Node 14 moves to maintenance mode in Oct 2021.

I'm not sure if it's just the node adapter or others as well, but module resolutions and bundling don't play well with each other nicely

By this do you mean the use of esbuild in the adapters? From what I understand reading the code, all adapters would have to output the adapt build output to ${SVELTE_KIT}/output/server/app.js, is that correct?

[ignatiusmb]

@JeanJPNM that's a good idea! It should be possible, it might need to be configured from the adapter's side to manually resolve the alias though

@jthegedus actually, I'm not sure either, but I know the node adapter is a special case since it's the only one that has a rollup config (begin doesn't count), and how it treats dependencies and devDependencies differently

[ignatiusmb]

This should now allow authors to import from @sveltejs/kit/app and call init and render.

An additional step has also been added to the list of things that an adapter should do, which is to import { appResolver } from '@sveltejs/kit/adapter' and pass it to the esbuild plugins for build options

[benmccann]

I don't think all adapters call esbuild, do they? Or at least it seems like it shouldn't be required if you don't need bundling. E.g. there's a ticket where esbuild causes a failure if you're trying to use TypeScript reflection, so perhaps we'd want to make it an option in adapter-node to allow people to turn it off

[ignatiusmb]

I don't see any other way to include the ../output/server/app.js other than to have a build step that bundles it, except adapter-static of course. I don't know about adapter-node though, but it's probably the most flexible of all. Maybe @jthegedus have any thoughts on this?

[benmccann]

adapter-node didn't have bundling for a long time. It was only added in #1648. I have mixed feeling about making it more required because I was thinking we should go the other way and make it optional as mentioned in the above comment

[benmccann]

These bullets should be "Call", "Convert", and "Globally shim" because you'd read them as a continuation of "Within the adapt method, an adapter should". Leaving off the "s" would also make it consistent with the first bullets which don't have an "s". Though this is a moot point if we add back "Output code that" because in that case it reads better with the "s"

[ignatiusmb]

Totally missed those! Agreed it would read better with the "Output code that" part, it might be better to make this its own section too as it's getting long.

[benmccann]

I liked having this line because it separates what the adapter itself is doing vs what is being done by the code that it outputs

[ignatiusmb]

I agree the separation is nice, but it looks awkward to have nested lists in the site. I don't remember Sapper or Svelte docs with this types of list, not visibly at least, and the styles doesn't look quite right. We can probably achieve better separation with headings and/or numbered list.

[JeanJPNM]

There is an issue with the modules and adapters that might affect this pr: adapters aren't getting the "full" types of the declared modules such as @sveltejs/kit/node or @sveltejs/kit/app.

If you hover over the init and render methods, you won't get any type information about them, in case of getRawBody it has a return type of any.

It seems that doing type Foo = import("./foo").Foo instead of import { Foo } from '@sveltejs/kit' fixes the issue, but it still makes me wonder why this happens on the first place.

[ignatiusmb]

@JeanJPNM I've had a couple of discussions with @dummdidumm as well about this and the results seems to vary each time. In my case, I don't see it either with declaring it as type alias (type Foo = ...), but we seem to agree that this works correctly with node_modules generated from package-lock.json. Can you also try and see if it works for you using package-lock.json, and if using a clean pnpm-lock.json vs linking it to a local copy of kit differs too?

[JeanJPNM]

By using package-lock.json you mean installing the dependencies with npm?

[ignatiusmb]

installing the dependencies with npm

Yes

[benmccann]

@ignatiusmb I left an idea for a path forward on this on the issue: #2249 (comment). Let me know what you think about that idea

[ignatiusmb]

Sure, replied in the issue thread, #2514 also gave me some more ideas.

I'll convert this to draft for now since we obviously need to rethink on the approach. Might also need to rewrite the docs because authors can technically still just point the import directly to the generated output and skipping the bundling step, but of course they would miss out on the typings which defeats the purpose yet again.

[benmccann]

Another idea might be to make the application a real module instead of relying on an esbuild plugin to resolve the location. Instead of writing the server output to .svelte-kit/output/server, we could try writing it to node_modules/@sveltejs/user-app (or possibly src/node_modules like Sapper does) with a package.json file:

		{
			"name": "@sveltejs/user-app",
			"version": "0.0.1",
			"private": true,
			"type": "module",
			"main": "app.js"
		}

[benmccann]

I think I'm going to go ahead and close this for now since it's probably not the direction we'd go in. I filed #2569 to track this as an issue. It might be easier to just start fresh with a new PR if we go with the node_modules approach, but feel free to reopen this if you'd rather update this one