feat: Add Typescript definitions

[theogravity]

I've handcrafted TS defs that is based on the API documentation and analysis of the project files.

We have had no problems using it in a project we're working on so far.

I'm unsure if you've worked with Typescript or not, but I've done my best to make sure that for return types where a class is involved, that an interface is returned instead.

This allows for the creation of a mocked class when writing unit tests, vs doing class definition overrides.

An exception case would be TokenSet as it doesn't do anything complicated and is pretty much a plain object under the hood.

[panva]

Whoa! @theogravity this is great, I will go through the PR to make sure nothing was missed soon.

[panva]

@theogravity i did a first pass review and identified a number of things that need fixing, in addition to those we neeed to add generators and the customization functionality, some of this is already part of the community maintained DefinitelyTyped.

I appreciate the effort you've put into this.

[panva]

dtto as jwks above.

[panva]

@ulrichb @YangusKhan @RMHonor as the three contributors to the DefinitelyTyped types, i would appreciate your review here, as well as believe its in your best interest since it would replace the types you've been maintaining.

[RMHonor]

Are you sure we want this extension here? We shouldn't be assuming there are additional properties on a client

[RMHonor]

As above, should an Issuer allow additional types like this?

[theogravity]

My linter fails if it's not included since it's implemented in IIssuer

[big-kahuna-burger]

Can we also have Strategy exported for the sake of completeness?

[panva]

I'd like someone to write .ts test file using the types so that we may have a CI job verifying them. Any takers?

[big-kahuna-burger]

I'd like someone to write .ts test file using the types so that we may have a CI job verifying them. Any takers?

@panva I will take it (via tsd). Expect PR later in the evening.

[Bessonov]

Great! While it's better to have definitions instead of not to have definitions at all... wouldn't it better to convert the project to typescript? What do you think @panva ? Maybe step by step.

I'd like someone to write .ts test file using the types so that we may have a CI job verifying them.

I'm not sure that this would help to check definitions. AFAIK you will check fictional[1] definitions with fictional[2] tests.

[1] fictional, because they can be not related to concrete implementation.
[2] fictional, because it doesn't make much sense to create high coverage tests just for checking typings.

Maybe it's better to convert all tests to ts as a first step?

[panva]

Let's park the tests for now until we're satisfied with the state of the definition.

@Bessonov exposing types is as far as I'm willing to go at this point in time.

[theogravity]

Thank you everyone for the quick turnaround!

I've addressed the most recent comments - apply JSONWebKeySet, and remove the DeviceFlowHandle def.

[panva]

@theogravity thank!

Re-iterating on the rest of missing pieces

In addition to those we neeed to add generators and the customization functionality, some of this is already part of the community maintained DefinitelyTyped.

And also the errors should probably be exported?

I appreciate the effort you've put into this.

already in place

[panva]

Thank you all for feedback and contributions. This has landed in 3.7.0

[ulrichb]

If the consuming project doesn't have @types/got installed, you get:

error TS7016: Could not find a declaration file for module 'got'.

=> Should we move @types/got from the dev-dependencies to the dependencies, so that TS compilation works out of the box?

[panva]

I intentionally did not do that, I don't see why non-ts users should have this as a dependency.

Maybe add a typescript readme section with this detail? Is that a big deal? Aren't ts users used to installing types? I assumed they are.

[RMHonor]

My first instinct was to put it in peer dependencies, as it then at least notifies the user. But perhaps optional dependencies would be more appropriate? Although it's not an approach I've encountered before

[panva]

@RMHonor neither of these two applies

optionalDependencies

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object.

peerDependencies

In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host.

[ulrichb]

@panva The (small) problem exists if you want to use node-openid-client, but don't use got in the consuming project. Then a) you first get the compile error mentioned above and b) you have to add an unnecessary @types\got package dependency in the consuming project.

Optional dependency looks like a good option :).

[panva]

Optional dependency looks like a good option :).

It isn't, it still installs for everyone. It just that it can optionally fail to install.

[panva]

you have to add an unnecessary @types\got package dependency in the consuming project.

How is it unnecessary? If you're using TS you need those types. If you don't you don't have to install it.

[RMHonor]

Something like optionalPeerDependencies would be perfect. I'd be inclined to say that it's fine to leave if as it is, and a small brief in the documentation around installing the 3rd party type definitions.

[ulrichb]

@panva:

How is it unnecessary?

If you don't use got directly in the consuming project (only indirectly via node-openid-client). Still without having @types\got installed you immediately get a compile error in this line:

https://github.com/panva/node-openid-client/blob/45402ede49375ed9bd9660f39d544f4483340bff/types/index.d.ts#L10

=> "error TS7016: Could not find a declaration file for module 'got'"

[panva]

In that case, i'm keen to remove the type dependency altogether and make it

{
  [key: string]: unknown;
}

and jsdoc add @see https://github.com/sindresorhus/got/tree/v9.6.0#options

[ulrichb]

Why is a normal dependency so bad? Packages often pull dependencies which aren't used in all cases / for all users.

An example: express has a dependency to cookie (or cookie-signature, or serve-static) which obviously isn't used by every express user.

[ulrichb]

Another argument: Also other packages to it that way.

For example react-intl (just an example; it was the first one I found in my project):

https://github.com/formatjs/react-intl/blob/899d17239545717283b1d539b011240428e9948d/package.json#L39-L40

[panva]

Why is a normal dependency so bad? Packages often pull dependencies which aren't used in all cases / for all users.

Packages often do, I choose not to. @types/got has three dependencies which also pack other transitive dependencies.

Showcasing bloated packages that do this is not an argument if i'm honest.

I was about to add the dependency then i checked what it means in terms of other transitive dependencies, so instead I'll move forward with the removal of that import altogether replacing it with the actual object with properties that make sense.

[panva]

v3.7.1 fixes this.

[ulrichb]

I'll move forward with the removal of that import altogether replacing it with the actual object with properties that make sense.

👍

[ulrichb]

Just tested it. Compiles fine without @types\got installed.

Many thanks @panva!

[theogravity]

Thank you everyone!

[RMHonor]

Thanks a lot @theogravity, really appreciate these definitions