Expose adapter init & render types for Adapter Authors

[jthegedus]

Describe the bug

Updating the Firebase adapter to the changes from #2215 was made easier by typing render, however finding the exact types for this was difficult. Once I found the type my refactoring was much easier and I even discovered a bug in my adapter for the Header types.

Expose the types.

Not sure if this constitutes a bug or feature request. I went bug because I think this is expected but not the current case.

Reproduction

You can see I have had to reference internal types @sveltejs/kit/types/internal to type this function:

/**
 * @param {import('firebase-functions').https.Request} request
 * @return {import('@sveltejs/kit/types/internal').Incoming}
 */
function toSvelteKitRequest(request) {

https://github.com/jthegedus/svelte-adapter-firebase/blob/c9c355eed0b26225ca4c549f7966aa695bccba37/src/files/handler.js#L29

I am not even sure I got the correct type.

Logs

No response

System Info

svelte-adapter-firebase on  fix/sync-kit-rawbody-changes [$⇕] is 📦 v0.10.4 via ⬢ v14.17.5 on ☁️  
➜ npx envinfo --system --binaries --browsers --npmPackages "{svelte,@sveltejs/*,vite}"
npx: installed 1 in 1.319s

  System:
    OS: Linux 5.11 Pop!_OS 21.04
    CPU: (8) x64 AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx
    Memory: 6.73 GB / 15.32 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 14.17.5 - ~/.asdf/installs/nodejs/14.17.5/bin/node
    npm: 6.14.14 - ~/.asdf/plugins/nodejs/shims/npm
  Browsers:
    Chrome: 92.0.4515.131
    Firefox: 91.0
  npmPackages:
    @sveltejs/kit: ^1.0.0-next.152 => 1.0.0-next.152

Severity

serious, but I can work around it

Additional Information

I believe exposing these types will make adapter development and refactoring more robust.

init is probably not necessary, though I do not know if it takes params and what they may be, so perhaps for visibility sake.

It would be good to just be able to force a type on

// TODO: hardcoding the relative location makes this brittle
import {init, render} from '../output/server/app.js';

[jthegedus]

Would be good to see all Adapter entrypoint types exported as well. For instance RequestHeaders in #2248

In jthegedus/svelte-adapter-firebase#123 I extracted the parsing of Headers into it's own function, and typed the return type as Record<string, string>, but would ideally use SvelteKit's RequestHeaders type directly.

[ignatiusmb]

I haven't really delved into adapters and its implementation, so your knowledge and other adapter authors are really appreciated here. Rephrasing one of my comment from our previous discussion -- Authors and users shouldn't need to reference anything outside the ones exposed publicly from @sveltejs/kit, especially internal.

This hasn't been a problem previously, why now and why does an adapter need access to Incoming? Do we need to expose init and render types or should the adapter change their behaviors to adjust with the recent changes? In case we do expose those types, would exposing the whole App type from internal makes more sense as it contains both of it in one package?

Again, I don't know much about adapters other than the fact that they have access to AdapterUtils and Config from the adapt function, and these are questions I genuinely have and would like to know more about. I also don't want to mess around too much with internal.d.ts if possible as that is mostly Rich's territory, moving them out of internal would mean that it should be fairly stable or used by others as well, and it would open up the possibility for that type to be exposed publicly when needed. But, that would also mean we're adding more to the API surface area, which we want to think carefully before doing so.

---

This is also a good time to start finalizing, or at least stabilizing the adapter API, so there should be as little breaking changes as possible. I'm thinking a dedicated @sveltejs/kit/adapter to have all the necessary APIs to build an adapter would be nice, everything packaged nicely in one place, how does that sounds like?

[jthegedus]

This hasn't been a problem previously, why now and why does an adapter need access to Incoming?

This issue has always existed, it just hasn't been raised because most people reviewing and contributing to the current adapters are the Kit core because most of the adapters live in the Kit monorepo.

Incoming is part of the Adapter API because the adapters must conform to this spec via App.render(). At the very least it should be considered a public part of the Kit API.

All adapters have some line like this in their files/entry.js:

// TODO hardcoding the relative location makes this brittle
import { init, render } from '../output/server/app.js'; // eslint-disable-line import/no-unresolved

I would like to apply a type to these imports to ensure what I write in my adapter conforms to these APIs. Thus far I have relied upon copying what is in Vercel adapter and external contributors.

Do we need to expose init and render types or should the adapter change their behaviors to adjust with the recent changes?

I would like to get type errors when updating my Kit dependency when Kit introduces a breaking change. I check most PRs going through Kit, but would like help should I miss something and would say the types would help.

In case we do expose those types, would exposing the whole App type from internal makes more sense as it contains both of it in one package?

App would be better, yes. However, how do I apply the type to render at usage time or import time? Additionally, when I want to extract some work into a function I would like to type the params and returns of the function. This is why I initially asked to expose AdapterUtils as well.

How would I type this usage of render and toSvelteKitRequest in the code below?

const rendered = await 	render(toSvelteKitRequest(request));

---

This is also a good time to start finalizing, or at least stabilizing the adapter API, so there should be as little breaking changes as possible. I'm thinking a dedicated @sveltejs/kit/adapter to have all the necessary APIs to build an adapter would be nice, everything packaged nicely in one place, how does that sounds like?

Sounds fine to me, though I would still argue App is part of the public spec.

[benmccann]

I put together a PR for this: #2259

[jthegedus]

I put together a PR for this: #2259

Thanks. Just need to figure out a way to get JSDoc to correctly apply the type to

import { init, render } from '../output/server/app.js';

---

Update 1: typing like this works, but is not ideal:

// @ts-expect-error
import * as App from '../output/server/app.js';

/** @type {import('@sveltejs/kit/types/internal').App} */
const app = App;

app.init();

Update 2: JSDoc types do not flow through functions, so exposing the Incoming type (as done in #2259) is a nicety to allow adapter Authors to extract some of the more complex parsing logic:

export default async function svelteKit(request, response) {
	// this call to app.render() should show a type error but does not
	const rendered = await app.render(toSvelteKitRequest(request));
	...
}

/**
 * Return not typed to allow bad type to be returned.
 * @param {import('firebase-functions').https.Request} request
 */
export function toSvelteKitRequest(request) {
	...
	return {
		wrong: 'type'
	}
}

[benmccann]

I believe that I was able to do it yesterday with something like this:

/** @type {import('@sveltejs/kit/app').App} */
import { init, render } from '../output/server/app.js';

[jthegedus]

Thanks for the new release 🙏

I know it is off-topic and user machine specific, but I am seeing mixed results with typing this code.

Typing on reassignment always works with func completions working and hover tooltips resolving the types properly:

// @ts-expect-error
import * as App from '../output/server/app.js';

/** @type {import('@sveltejs/kit/types/internal').App} */
const app = App;

Typing the import as you suggest gives me no func autocomplete, typing when applying braces, and then no types on hover after completely written.

And finally, de-sctructuring at import just gives me any for everything as if untyped:

/** @type {import('@sveltejs/kit/types/internal').App} */
import {init, render} from '../output/server/app.js';

// init and render show as "any"

Anyway, thanks for exposing these more easily.

[ignatiusmb]

Exporting App should make this a bit easier now than directly referencing the internal types, but I think we can do better. We cannot re-type imports, so authors would need to redeclare app and manually type them

// @ts-expect-error
import * as App from '../output/server/app.js';

/** @type {import('@sveltejs/kit').App} */
const app = App;

I'll try to open a draft for proxying the init and render imports, so we should be able to use them like

import app from '@sveltejs/kit/adapter';
// or alternatively
// import { init, render } from '@sveltejs/kit/adapter';

But, there's also a lot to do with the current adapter implementations as well, so reopening to keep track of this for now.

[benmccann]

I wonder if in build/index.js where we write the app whether writing a JSDoc to type it might help? I.e. add a JSDoc on the line below (and also for render) so that consumers don't need to be responsible for adding the type:

kit/packages/kit/src/core/build/index.js

Line 317 in aaea5cf

export function init(settings = default_settings) {

Nevermind. That won't work because as an adapter author the app hasn't been built yet

[ignatiusmb]

That might be a good idea, but I'm hoping we could also resolve this TODO while we're at it

kit/packages/adapter-node/src/index.js

Lines 1 to 2 in aaea5cf

	// TODO hardcoding the relative location makes this brittle
	import { init, render } from '../output/server/app.js'; // eslint-disable-line import/no-unresolved

[jthegedus]

I guess this is now related to #625

[benmccann]

Potentially there'd be some way to do this by refactoring so that instead of the whole app being written at build-time it's already provided like any typical module. Then we could pass in the options and manifest (maybe to init) since those are the dynamically generated portions that do need to be created at application build time. We might also have to also change init to be async because we might have to dynamically include user_hooks. It'd probably also help clean up duplication between dev and build, which would be nice as well

[ignatiusmb]

Interesting take, making it a module might be the way to go for providing the types. But, if we're still keeping some portion dynamically generated, what would be a way to get those generated chunks and pass them to the now modularized imports? We need some way to avoid imports like #2249 (comment) for getting the generated portions

[benmccann]

I filed #2569 to track fixing the relative path, which is the last piece needed. The discussion here was getting a bit long and encompassed a wider variety of stuff, so maybe cleaner to start with a fresh issue for that