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

add out option to adapter-node, and add types for adapters #531

Merged
merged 4 commits into from
Mar 16, 2021

Conversation

Rich-Harris
Copy link
Member

Makes the output directory for adapter-node configurable. While I was at it, I added an Adapter type, but haven't yet applied it to the other adapters.

This entailed moving some types out of kit/src/types.d.ts into kit/types.internal.d.ts so that they can more easily get included in the package, which makes the PR look much bigger than it is.

@@ -1,3 +1,5 @@
import { Logger } from './types.internal';
Copy link
Member

Choose a reason for hiding this comment

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

It seems like Logger isn't really an internal type if it's being exposed here. Why not just put it in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's internal insofar as you're not going to do

/** @type {import('@sveltejs/kit').Logger} */

— it's always just a property of the builder (which also needn't be exported) that's passed to the adapt function. this file should only export types that need to be consumed by users of Kit

* out?: string;
* }} options
*/
export default function ({ out = 'build' } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

should we have an interface for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

nah, it's adapter-specific

@@ -1,8 +1,8 @@
/**
* @param {import('../../types').Request} request
* @param {import('../../../types.internal').Request} request
Copy link
Member

Choose a reason for hiding this comment

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

I would expect that Request and Response should not be internal because almost every adapter will have to deal with them

Copy link
Member Author

Choose a reason for hiding this comment

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

probably. we can move things out of internal as and when necessary

@Rich-Harris
Copy link
Member Author

Tests are failing but I'm pretty confident it's entirely due to flakiness rather than anything in this PR, so I'll merge this

@Rich-Harris Rich-Harris merged commit 9212aa5 into master Mar 16, 2021
@Rich-Harris Rich-Harris deleted the adapter-node-options branch March 16, 2021 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants