types: Use Request from TS built-in types

outloudvi commented 2 years ago

This PR replace the written Request with Request from TypeScript lib dom.

Why the triple-slashes?

For declaration file authors who rely on built-in types, e.g. DOM APIs or built-in JS run-time constructors like Symbol or Iterable, triple-slash-reference lib directives are recommended. (source)

Why the stock `Request` will work?

The Request interface represents an HTTP request and is part of the Fetch API. (source)

To test the types...

@ovv/itty-router

Related: #57

bever1337 commented 2 years ago

Should the typings also reference the three non-standard properties from itty's request mutation?

interface IttyRequest {
  params?: {
    [key: string]: string | undefined;
  };
  proxy?: Proxy;
  query?: {
    [key: string]: string | undefined;
  };
}

export type Foobar = Request & IttyRequest;

outloudvi commented 2 years ago

I've added these properties:

params: lib.es2018.regexp.d.ts#L22-L24
query: I think we can assume it in the way mentioned on the last comment
proxy: TypeScript doesn't specify anything other than it extending an object.

Onto #67, I agree that params can somehow be a somehow multimap due to the property of URL parameters, but I think the typing shall only be updated if the source js has changed.

Feel free to check things out at @ovv/itty-router@2.6.1-trunk-d99a4c4.

bever1337 commented 2 years ago

@outloudvi did you see this comment from the owner? https://github.com/kwhitley/itty-router/issues/67#issuecomment-1118950351

I came to this project trying to use standard Request+Response pairs (seemingly like you did) and misunderstood that is intentionally outside of itty's scope. This is just a guess, but I suspect this typing PR moves against itty's established direction. For example, Node express servers won't operate on request+response pairs. That said, nothing about Itty's internal typings has prevented me from using standard request+response objects in my handlers!

outloudvi commented 2 years ago

This is just a guess, but I suspect this typing PR moves against itty's established direction.

I've read the comment and I think that would make sense.

I brought up this PR because it says it's "built for Cloudflare Workers" in the README, but the typing lacks some basic properties I need (in my case, headers) when I'm using it for a Workers project (sorry Worktop, but I failed to understand how I can make nested routes there)*. If I don't do any workaround on my side of propose a change on the upstream side, tsc will refuse to compile the code, even if it actually works as JS after scrubbing all the type notations.

The Router function in the original d.ts is a generic function, so I would say it is pretty extensible (just throw whatever you want to the generic TRequest parameter). And if this package is meant for a wider range of networking stuffs (for example, the ones that don't contain the headers property in Request), then yes, the PR is apparently against itty's established direction.

As of the request.query thing, I would say the d.ts file actually has nothing to do with any sort of spec. There is only one simple rule: if the JavaScript code returns an Obj, the d.ts is supposed to annotate the return value as an Obj.

* Actually, I just realized that the cf property is still missing in the PR if the PR is meant to fix Workers developers' experience.

jacwright commented 1 year ago

Actually, I just realized that the cf property is still missing in the PR if the PR is meant to fix Workers developers' experience.

I don't think you need to add the cf property in a workers environment. When you install @cloudflare/workers-types for development those definitions add cf to the native Request definition. See https://github.com/cloudflare/workers-types/blob/bf5d870b4e1466291c3cbdcba3001ab28f3ea400/overrides/cf.d.ts#L3-L5

outloudvi commented 1 year ago

I don't think you need to add the cf property in a workers environment.

Yup, I guess you're right - that shall have been already covered by @cloudflare/workers-types. Thanks!

tgriesser commented 1 year ago

FYI, just opened https://github.com/kwhitley/itty-router/pull/118 which overlaps with this a bit, would appreciate any feedback on the changes!

kwhitley commented 1 year ago

Actually, I just realized that the cf property is still missing in the PR if the PR is meant to fix Workers developers' experience.

I don't think you need to add the cf property in a workers environment. When you install @cloudflare/workers-types for development those definitions add cf to the native Request definition. See https://github.com/cloudflare/workers-types/blob/bf5d870b4e1466291c3cbdcba3001ab28f3ea400/overrides/cf.d.ts#L3-L5

Good catch - this needs to be covered somewhere in the docs... ideally we'll have a Quick Start guide for various environments (including TS vs JS), and certainly for Workers, as that's the primary (but not only) audience.

kwhitley commented 1 year ago

Closing this PR for now, as the file is no longer part of the codebase (v3.x is out, as a TS-first library).

I'll only add that in my experience, just adding to my tsconfig has been plenty in most cases!

"types": [
      "@cloudflare/workers-types"
],

kwhitley / itty-router