[feat] adapter module #2285

ignatiusmb · 2021-08-25T02:45:34Z

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 · 2021-08-25T02:45:36Z

⚠️ 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 · 2021-08-25T03:40:44Z

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

…-module

ignatiusmb · 2021-08-25T15:52:13Z

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 · 2021-08-26T13:59:30Z

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

jthegedus · 2021-08-31T03:31:40Z

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 · 2021-08-31T06:00:54Z

@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

…-module

ignatiusmb · 2021-09-02T06:45:26Z

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

packages/kit/src/core/adapt/plugins.js

packages/kit/types/ambient-modules.d.ts

benmccann · 2021-09-02T15:15:47Z

documentation/docs/80-adapter-api.md

-  - Converts from the platform's request to a [SvelteKit request](#hooks-handle), calls `render`, and converts from a [SvelteKit response](#hooks-handle) to the platform's
-  - Globally shims `fetch` to work on the target platform. SvelteKit provides a `@sveltejs/kit/install-fetch` helper for platforms that can use `node-fetch`
+- Import `{ appResolver }` from `@sveltejs/kit/adapter`
+- Pass in `appResolver()` to esbuild plugins for build options


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

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?

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 · 2021-09-02T15:17:19Z

documentation/docs/80-adapter-api.md

+- Import `{ appResolver }` from `@sveltejs/kit/adapter`
+- Pass in `appResolver()` to esbuild plugins for build options
+- Import `app` from `@sveltejs/kit/app`
+- Calls `app.init()`


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"

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.

documentation/docs/80-adapter-api.md

benmccann · 2021-09-02T16:00:59Z

documentation/docs/80-adapter-api.md


 - Clear out the build directory
- Output code that:


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

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 · 2021-09-03T06:39:55Z

@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 · 2021-09-06T20:35:07Z

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

ignatiusmb · 2021-09-13T07:47:09Z

installing the dependencies with npm

Yes

benmccann · 2021-09-28T15:12:38Z

@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 · 2021-09-30T02:46:54Z

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 · 2021-10-02T12:31:10Z

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 · 2021-10-07T23:25:42Z

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

ignatiusmb added 4 commits August 25, 2021 09:16

create proxy for adapters to import from

cbfd2cf

adapter ambient module

f94fbe8

adapter node sync

11b99fc

update constants

8fde219

ignatiusmb added 5 commits August 25, 2021 10:14

add to exports

f9bff84

promisify module export types

09d007c

properly type default export

a76784b

shorter namings

17a9bbe

revert some paths

409b239

Merge branch 'master' of https://github.com/sveltejs/kit into adapter…

09cf6a9

…-module

ignatiusmb added 9 commits September 2, 2021 12:56

tidy up imports

6f50f5b

@sveltejs/esbuild-plugin-app-resolver

b07e677

allow importing app output from @sveltejs/kit/app

f069539

Merge branch 'master' of https://github.com/sveltejs/kit into adapter…

61741f1

…-module

resolve merges

4abc0a2

use appResolver for adapter-node

6cb5be2

update adapter api docs

8fd0c4c

lockfile

e0602fc

adapter node tests

e84a688

ignatiusmb added 3 commits September 2, 2021 13:50

revert constants changes

fa20353

remove promise wrapper from types

88d541a

update rest of adapter import paths

e1af57c

ignatiusmb marked this pull request as ready for review September 2, 2021 06:59

jthegedus reviewed Sep 2, 2021

View reviewed changes

packages/kit/src/core/adapt/plugins.js Show resolved Hide resolved

JeanJPNM suggested changes Sep 2, 2021

View reviewed changes

packages/kit/types/ambient-modules.d.ts Outdated Show resolved Hide resolved

remove accidental type as callback

d44975b

benmccann reviewed Sep 2, 2021

View reviewed changes

JeanJPNM reviewed Sep 2, 2021

View reviewed changes

some wording

9dff930

Merge branch 'master' into adapter-module

9ab01b5

fix merge conflict

15f6149

ignatiusmb marked this pull request as draft September 30, 2021 02:47

benmccann mentioned this pull request Oct 3, 2021

Source maps in adapter-node #2517

Closed

benmccann closed this Oct 7, 2021

ignatiusmb deleted the adapter-module branch February 3, 2022 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] adapter module #2285

[feat] adapter module #2285

ignatiusmb commented Aug 25, 2021 •

edited

changeset-bot bot commented Aug 25, 2021 •

edited

benmccann commented Aug 25, 2021 •

edited

ignatiusmb commented Aug 25, 2021

JeanJPNM commented Aug 26, 2021 •

edited

jthegedus commented Aug 31, 2021

ignatiusmb commented Aug 31, 2021

ignatiusmb commented Sep 2, 2021

benmccann Sep 2, 2021 •

edited

ignatiusmb Sep 3, 2021

benmccann Sep 13, 2021

benmccann Sep 2, 2021

ignatiusmb Sep 3, 2021

benmccann Sep 2, 2021

ignatiusmb Sep 3, 2021

JeanJPNM left a comment •

edited

ignatiusmb commented Sep 3, 2021

JeanJPNM commented Sep 6, 2021

ignatiusmb commented Sep 13, 2021

benmccann commented Sep 28, 2021

ignatiusmb commented Sep 30, 2021

benmccann commented Oct 2, 2021 •

edited

benmccann commented Oct 7, 2021

[feat] adapter module #2285

[feat] adapter module #2285

Conversation

ignatiusmb commented Aug 25, 2021 • edited

changeset-bot bot commented Aug 25, 2021 • edited

⚠️ No Changeset found

benmccann commented Aug 25, 2021 • edited

ignatiusmb commented Aug 25, 2021

JeanJPNM commented Aug 26, 2021 • edited

jthegedus commented Aug 31, 2021

ignatiusmb commented Aug 31, 2021

ignatiusmb commented Sep 2, 2021

benmccann Sep 2, 2021 • edited

Choose a reason for hiding this comment

ignatiusmb Sep 3, 2021

Choose a reason for hiding this comment

benmccann Sep 13, 2021

Choose a reason for hiding this comment

benmccann Sep 2, 2021

Choose a reason for hiding this comment

ignatiusmb Sep 3, 2021

Choose a reason for hiding this comment

benmccann Sep 2, 2021

Choose a reason for hiding this comment

ignatiusmb Sep 3, 2021

Choose a reason for hiding this comment

JeanJPNM left a comment • edited

Choose a reason for hiding this comment

ignatiusmb commented Sep 3, 2021

JeanJPNM commented Sep 6, 2021

ignatiusmb commented Sep 13, 2021

benmccann commented Sep 28, 2021

ignatiusmb commented Sep 30, 2021

benmccann commented Oct 2, 2021 • edited

benmccann commented Oct 7, 2021

ignatiusmb commented Aug 25, 2021 •

edited

changeset-bot bot commented Aug 25, 2021 •

edited

benmccann commented Aug 25, 2021 •

edited

JeanJPNM commented Aug 26, 2021 •

edited

benmccann Sep 2, 2021 •

edited

JeanJPNM left a comment •

edited

benmccann commented Oct 2, 2021 •

edited