Adding basic type for typescript support

ssanchezmarc commented 3 years ago

Context

Considering the issue https://github.com/hapijs/jwt/issues/30 to add type support, I have started the work defining the very very basic type required to avoid ts linter error when you import the plugin in your Hapi solution.

I know that it is still pending the definition of the rest of modules, but at least it allows the basic use with typescript support.

As a reference, I have used type definition for Hapi cookie plugin

Work done

[x] Plugin type definition for the main module
[x] Skeleton for testing types and coverage test for the ones added in the PR

Alternatives

We can define the types into Definitely Typed project but I have considered that if we give support internally, it will always be the best option in terms of maintainability, and users will not have to install additional dependencies. Anyway, no worries if you consider that use Definitely Typed here is a better option 😉 it is just a matter of moving it

devinstewart commented 3 years ago

@ssanchezmarc: due to the fact that there is not a whole bunch here, I personally would like to keep it internal as opposed to relying on Definitely Typed.

Are you willing to continue to contribute on this effort?

ssanchezmarc commented 3 years ago

@ssanchezmarc: due to the fact that there is not a whole bunch here, I personally would like to keep it internal as opposed to relying on Definitely Typed.

Are you willing to continue to contribute on this effort?

Hi @devinstewart,

My initial intention was to make this PR to start type definitions with a small effort, at least with this, the ts linter does not complain, but I understand that we prefer to complete the full type definition before delivering a new version of the plugin, right?

I will try to find some space to complete the contribution as soon as I can 😉 I promise. Let's say, I am not an expert on types definition with typescript, but it could be a good exercise for me to practice. In the meanwhile, if someone wants to continue the work, feel free of course.

ssanchezmarc commented 3 years ago

I have changed the PR to draft PR as it is still in progress. Pending to define the types for additional token functionality

kanongil commented 3 years ago

I don't think it is appropriate for a hapijs module to rely on the unsupported typings in @types/hapi__hapi. I think this should probably go in Definitely Typed for the time being.

ssanchezmarc commented 3 years ago

I think this should probably go in Definitely Typed for the time being.

Hi @kanongil,

Actually, I have followed the same approach we follow in other Hapi plugins, for instance, hapi-auth-jwt2, but I understand your point.

Considering that Definitely Typed is very adopted by the community, I would assume the tradeoff. I prefer the benefits of having here the types close to the code even it supposes to use a no official Hapi repo.

BTW, I will accept the decision we make 😉 moving this to Definitely Typed is straightforward task

kanongil commented 3 years ago

As an hapijs org repo, this has to follow the standards of the same. Not some random external module.

ssanchezmarc commented 3 years ago

As an hapijs org repo, this has to follow the standards of the same. Not some random external module.

@kanongil do you see any other option here? maybe there is a third option where we can keep here and avoid consuming Definitely Types repo

If not, considering @kanongil opinion, @devinstewart @Nargonath should we close this PR? I could make the work to move it to Definitely Types repo. No strong opinion here from my side, you have more experience than me working here, so I have no principles for now 😛

devinstewart commented 3 years ago

There are other modules that have fully contained TS. (wreck and boom are two examples). However, the difference with those guys and all the others I've seen are not Plugins, so there is not a dependency on hapi itself. Which uses Definitely Type (not maintained by @hapijs).

So... While I would love to keep things local so we can control them, if this is a blocker, I guess Definitely Type is the way to go.

ssanchezmarc commented 3 years ago

@devinstewart @Nargonath @kanongil I have already moved the types definition to Definitely Typed project. Here the opened PR https://github.com/DefinitelyTyped/DefinitelyTyped/pull/50945. So we can close this PR.

Right now, I am the only owner of that. If any of you want to be as an owner too, please let me know I can modify the owner list to include you 😉

I hope you find these definitions useful. Let see if they are merged in the DefinitelyTyped project to be available for the community 🥳

Kind regards, Sergio

Nargonath commented 3 years ago

@ssanchezmarc Thank you for the work. I didn't have time to follow through the whole thing but you guys seem to have handled it well. I agree with you that the best would be to have the typings within the org but right now we don't have the bandwidth to do so. For now relying on DT is an acceptable solution. 👍

hapijs / jwt