panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

Missing `register` function signature in `Issuer` #232

Closed alvis closed 4 years ago

alvis commented 4 years ago

It only affects typescript users.

Currently, if you create an issuer, typescript would complain if you use the client register function. e.g.

const issuer = new Issuer();

issuer.Client.register();

It's due to the type definition on Issuer that the type definition of Client is not typeof Client. Therefore, all the static methods are missing. See below:

https://github.com/panva/node-openid-client/blob/dd2194e8680af9a9b996a1973618999e1d821f01/types/index.d.ts#L517-L523

https://github.com/panva/node-openid-client/blob/dd2194e8680af9a9b996a1973618999e1d821f01/types/index.d.ts#L507-L511

panva commented 4 years ago

@alvis can you propose a fix PR to the types? In addition i noticed the types actually suggest that the Client class is exported when it isn't.

panva commented 4 years ago

ping @theogravity who contributed the types in the first place.

theogravity commented 4 years ago

Register is defined here:

https://github.com/panva/node-openid-client/blob/dd2194e8680af9a9b996a1973618999e1d821f01/types/index.d.ts#L456

Did you try specifying the Client as the generic impl?

const issuer = new Issuer<Client>();

Might be possible to not have to do this by specifying a default for the generic def in the definitions:

https://github.com/panva/node-openid-client/blob/dd2194e8680af9a9b996a1973618999e1d821f01/types/index.d.ts#L517

export class Issuer<TClient extends Client = Client>

Try that out and issue a PR if it fixes it.

panva commented 4 years ago

Okay, couple of issues i'd like to see a PR for

1) DeviceFlowHandle is not exported by the library but is in the types 2) Client is not exported by the library but is in the types 3) make sure the Client methods mentioned here are available by default

theogravity commented 4 years ago
  1. We actually use the type info from DeviceFlowHandle even if it's not exported (eg. in tests, or it's returned by some other lib call).
  2. You still need the typing info for it, since it's accessible in other ways (like the OP's issue); we also use it in our project
  3. it is already in there as noted in my response above
panva commented 4 years ago

it is already in there as noted in my response above

until the following code passes type checks i wouldn't call it "there"

const issuer = new Issuer();

issuer.Client.register({});

You still need the typing info for it

type Client = InstanceType<Issuer['Client']>;

I'd expect the following to work to get your typing info. And DeviceFlowHandle can easily be an exported interface that DeviceFlowHandle implements.

theogravity commented 4 years ago

Right, which is why I said this prob fixes it (it would use the Client interface as the default):

export class Issuer<TClient extends Client = Client>

panva commented 4 years ago

It doesn't from what i was able to test.

theogravity commented 4 years ago

I'll take a look

panva commented 4 years ago

Thank you Theo

panva commented 4 years ago

Closing this as stale, feel free to come up with a PR to unblock this mess.

panva commented 4 years ago

@theogravity poke