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]

@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.

[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.

[theogravity]

That should complete the definitions!

I based the error defs on what I saw here: https://github.com/panva/node-openid-client/blob/master/lib/errors.js

I did some refactoring based on the DefinitelyTyped defs.

For the generator / custom types, unsure which to use for it (export namespace or export const). If someone can advise me on when to use one over the other for exporting an object, that'd be great. In the end, I did what the DT defs were doing.

Feel free to update it if it looks incorrect in any way

Note that I added [key: string]: unknown to the Client and Issuer classes because the interfaces also define it. If this is incorrect, remove it from both the class and interface please.

[theogravity]

The CI needs a kick on one of them - been stalled for hours on npm install

[panva]

I’ll retrigger with an empty push, UI doesn’t seem to work all that well ;)

[panva]

One more thing - the inline docs in the ts definition - does it show up in intellisence for developers? In my experience it doesn't so i'd rather remove it and only have regular docs to avoid having to maintain two places?

[theogravity]

Just realized - I was testing in my own project, but since this is to be included in yours, the module declaration isn't necessary.

I was able to get comments in VS Code after doing that. Try again.

If you don't get the got options, try npm i @types/got --save-dev

[panva]

@ulrichb @YangusKhan @RMHonor @big-kahuna-burger @Bessonov

I'm looking for more ✅ from you guys, please npm i theogravity/node-openid-client#typescript-defs and check if this fits to replace @types/openid-client

[big-kahuna-burger]

@ulrichb @YangusKhan @RMHonor @big-kahuna-burger @Bessonov

I'm looking for more ✅ from you guys, please npm i theogravity/node-openid-client#typescript-defs and check if this fits to replace @types/openid-client

LGTM, Strategy is in which I wished for. Thanks @theogravity 🚀

[ulrichb]

... also unit test (+ execution in the build script) would be cool.

As a starting point you could use these: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/openid-client/openid-client-tests.ts

[panva]

... also unit test (+ execution in the build script) would be cool.

As a starting point you could use these: DefinitelyTyped/DefinitelyTyped:types/openid-client/openid-client-tests.ts@master

@ulrichb can you elaborate on the steps necessary to execute this?

[theogravity]

Based on reading the DefinitelyTyped repo:

You write a series of statements, such as:

const client = new Client(...)

And you write enough of them that it would cover the types that you've written in your definition, then you execute a TS linter against it. If the linter passes, then it's all good.

So what I'm guessing here is if you want to actually have the CI test the defs file, is you write a sample .ts file and run the TS linter on it.

https://github.com/palantir/tslint is what a lot of people use, including myself. The project is migrating to eslint, though.

If you use standard, there's eslint plugins for ts as well:

https://standardjs.com/#typescript

[theogravity]

Let me know if this is something you'd like me to take the lead on, I'd be happy to set it up, just didn't want to mess with your dependencies if possible.

[panva]

I refactored the types, removed a lot of the clutter and added the test to CI (part of the Lint job).

Please take a look - @big-kahuna-burger i made changes to the strategy definition, can you please verify it still works as intended for you? I didn't bother with the tests yet.

[theogravity]

Since you've removed the interfaces that the classes implement (I understand why), I would really recommend that anywhere a class is defined as part of a property, parameter, return, etc, that a generic be used. It would make life so much easier when unit testing as I'd be able to create my own custom class and easily override any methods that I need to mock.

Not only that, if I end up writing a custom class that extends it and add new methods, typescript would be able to provide autocompletion support, refactor capabilities, and would recognize my new method because I can specify my custom class as the type for the generic

class Issuer <C extends Client> {
     client: C

     constructor(client : C)

     getClient (): C
}

function blah<C extends Client, I extends Issuer>(client: C, issuer: I) {

}

class CustomIssuer extends Issuer<CustomClient> {  
   myCustomFn() : CustomClient {

   }
}

function blah<Client, CustomIssuer>(new Client(), new CustomIssuer()) {
   // if not using generics, TS compilation would fail if the issuer != instanceof(Issuer) 
   // and also referencing a method that does not exist on the Issuer class
   // with generics, typescript will be able to acknowledge the new method
   issuer.myCustomFn()
}

[panva]

@theogravity as-is now i can not only understand the types but first and foremost can continue maintaining them. Not using TS myself I don't get the samples you're showing me.

Furthermore the types that the community published to DefinitelyTyped are closer to what the state is now and it seems to be enough.

What do others think?

[theogravity]

I'm unsure if your comment was directed towards my comment about introducing back the interface, but I deleted the comment because I realized that maintainability / duplication would be an issue over any usefulness it would provide.

a compromise that still works well is using generics instead - what are your thoughts on generics? they're not difficult to implement, maintenance overhead is extremely low, and adds a lot of flexibility

[panva]

It is directed at your comment. "generics" has no meaning to me as non-TS user. If you want to show me please make the update. But no promises, it'll stick :)

[theogravity]

Understood. Will do it right now.

[panva]

Understood. Will do it right now.

no rush, i ain't got time today to look into this anymore.

[theogravity]

No problem. Thanks for refactoring it.

I've added generics where it made sense to. Hopefully the tests demonstrates how powerful it can be.

I had to add the types package for passport-strategy and got as VS Code was not recognizing the types at all until I added them.

[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