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

Even more type improvements #146

Closed
wants to merge 2 commits into from
Closed

Even more type improvements #146

wants to merge 2 commits into from

Conversation

enisdenjo
Copy link
Collaborator

@enisdenjo enisdenjo commented Sep 26, 2022

  • Remove extraneous generics, they can confuse TS and mess with inference
  • ServerAdapter implements requestListener too
  • NodeServerAdapterContext (previously DefaultServerAdapterContext) is not the default context - it is the context only when using Node request handler. Shouldn't be used as default for context generic.

@enisdenjo enisdenjo requested a review from ardatan September 26, 2022 22:12
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2022

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@whatwg-node/fetch 0.4.6-alpha-20220927131110-4cfdcdb npm ↗︎ unpkg ↗︎
@whatwg-node/server 0.4.10-alpha-20220927131110-4cfdcdb npm ↗︎ unpkg ↗︎

@@ -62,9 +58,9 @@ export interface ServerAdapterObject<
): Promise<Response> | Response;
}

export type ServerAdapter<TServerContext, TBaseObject extends ServerAdapterBaseObject<TServerContext>> = TBaseObject &
Copy link
Owner

@ardatan ardatan Sep 27, 2022

Choose a reason for hiding this comment

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

Are we still able to keep other methods of given object in the returned object?

const app = createServerAdapter(Router());
app.get('/', req => ...);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, so that is the reason why we need the TBaseObject generic.

The thing is, with the current setup (on master), inference does not work with the context. Try this:

interface Ctx {
  some: string;
}

const handler = (_req: Request, _ctx: Ctx) => new Response();
const adapter = createServerAdapter(handler); // TS error with handler

const request = new Request('http://localhost');
await adapter(request, ctx); // TS error with request

even passing in the Ctx as generic creates problems:

- const adapter = createServerAdapter(handler);
+ const adapter = createServerAdapter<Ctx>(handler);

@ardatan ardatan closed this Mar 14, 2023
@ardatan ardatan deleted the types branch June 13, 2023 09:07
@theguild-bot theguild-bot mentioned this pull request May 23, 2024
15 tasks
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