Definitelytyped: Major hapi review, refactor and update to 16.1.1

[AJamesPhillips]

I've completed updating the types for hapi 16.1.1 and some of the plugins. They're now available here: https://github.com/AJamesPhillips/DefinitelyTyped/tree/hapi-v16.1/types/hapi
I made a mistake whilst updating the inert plugin types before: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/15616/commits/b33942b17b4117f2fa5b2eae6ad3baa55323768f so I'm only going to submit these new types to the repository when I have tested them for a week or so. If anyone else who uses Hapi with TypeScript could test these with real production code and suggest any improvements that'd obviously be very useful.

There are a large number of changes. In terms of backwards compatible the major breaking change (vs the existing tests now in the v16.0 directory) is the lack of IReply. From the docs it seemed there were many subtly different reply(...) methods some with and without reply.continue()... so these have become IReplyWithContinue and IReplyNoContinue. Also I didn't put back in the generic IReply known as IStrictReply, did people find this useful? The DefinitelyTyped Readme under common mistakes talks about not using generic types unless the type was used in typing one or more of the function arguments. Using it to type the return was suggested to be the same as a type assertion. Can obviously put back in if needed:

getMeAT<T>(): T: If a type parameter does not appear in the types of any parameters, you don't really have a generic function, you just have a disguised type assertion. Prefer to use a real type assertion, e.g. getMeAT() as number. Example where a type parameter is acceptable: function id<T>(value: T): T;. Example where it is not acceptable: function parseJson<T>(json: string): T;. Exception: new Map<string, number>() is OK.

There are also concurrent changes to inert included within the branch above, as well as PR to Hapi regarding some of the documentation.

@corbinu @MeirionHughes @eobrien-fsp @mmc41 @jasonswearingen
Thanks.

[jasonswearingen]

Hi @AJamesPhillips it looks like you put a lot of work into this! I am in the middle of my projects now so I can't really try out the revised typings, but I'm happy you spent the time to flesh out the various sub-module dependencies!

[mmc41]

@AJamesPhillips @jasonswearingen If we are updating @types/hapi in a major way, I wonder if it would make sense to introduce two new types to help create hapi plugins/modules in a type safe manner. In my own project I have created these two types on the side, but I think it could make sense to declare them in @types/hapi (possibly after a rename):

export interface HapiModuleFunctionAttributes {
   name: string;
   version: string;
   multiple?: boolean;
};

export interface HapiModuleFunction<O> {
     (server: Server, options: O, next: (err?: any) => void): void;
     attributes?: HapiModuleFunctionAttributes;
}

They can than be used when declaring an plugin like this:

export const register: HapiModuleFunction<MyOptionsType> = function (server, options, next) {
...
}

register.attributes = {
   name: 'uiPlugin',
   version: '1.0.0'
};

Let me know what you think?

[AJamesPhillips]

@mmc41 Thanks for the input. re HapiModuleFunctionAttributes, yes, this is good. Currently it's here: https://github.com/AJamesPhillips/DefinitelyTyped/blob/5aed938/types/hapi/index.d.ts#L2434
Current implementation has two main differences from yours, firstly that it's not generic and just has options: OptionsPassedToPlugin instead of options: O where type OptionsPassedToPlugin = any. And secondly that attributes is not optional meaning you have to declare it using a cast: https://github.com/AJamesPhillips/DefinitelyTyped/blob/5aed93802ab1019305671721422a6d4e09fc5286/types/hapi/tests/plugins/options.ts#L29 I will leave as it is unless you have any changes you think we'd benefit from including.

From your comment on the hapi issue, could you point me to the docs I'm missing please. I tried my best to include everything from them but might have missed some bits. Thanks.

Regarding

Also, I noticed you are missing types for @types/chai in your repo?

Sorry I'm not sure what you mean

• https://github.com/AJamesPhillips/DefinitelyTyped/tree/5aed93802ab1019305671721422a6d4e09fc5286/types/chai
• https://hapijs.com/api/16.1.1

[jasonswearingen]

I am okay with merging this into DT proper, just maybe add a comment at the top informing of breaking changes and telling people to use an older definition if they want to avoid refactoring.

[mmc41]

@AJamesPhillips You are missing the following updated type definitions from v16.0.1 (if you do a rebase you will see them):

export declare type IThenable<R> = PromiseLike<R>
export declare type IPromise<R> = Promise<R>

I think the current IPluginFunction declaration is problematic because it removes type safety for plugins. A simple change would be to rewrite it as below and remove the (assignment of any to OptionsPassedToPlugin in the current code:

export interface IPluginFunction< OptionsPassedToPlugin> {
    (server: Server, options: OptionsPassedToPlugin, next: (err?: Error) => void): void;
}

P.S. Also, if I look at the example for how to use IPlugin to register attributes, I think my definition make it much easier in comparison. What about updating both but keep the current names?

[mmc41]

@AJamesPhillips As for what I mean with chai missing, is that if I look at "https://github.com/AJamesPhillips/DefinitelyTyped/tree/hapi-v16.1/types/hapi" that you refer to, I see lots of folders for various key hapi components - except I see no folder for chai. Hence, I was just wondering if it was missing ?

[AJamesPhillips]

@mmc41 With regard to the typings you suggested, I've made the IPluginFunction generic as you suggested and it does make the Inert definition more complete (we now have the registration options as an interface). It adds a bit of complexity to the typings but for plugins with required registration option key values, it would make it more robust. So good idea, thank you.

With regard to using a single interface with an optional attributes. I've done this as well as you're right that it does make it easier to use. It does removes type safety as attributes is not optional but being that you had to do a non type safe casting step with the current definitions anyway we haven't lost anything and just gained simplicity. Removing a cast always makes things happier. Thanks for that suggestion too.

Regarding IPromise etc, I saw the issue raised at #15481 and the PR with #15568 but the later PR by @andy-ms #15756 correctly reworks all the return types to just use Promise instead of IPromise albeit with backwards incompatibility (as you pointed out in 15481). I would be happy to add back in under a 'backwards compatibility' section but it's not hard to update and we've potentially broken (fixed? :) ) quite a lot of code anyway.

Regarding Chai. Ah I see, no I just went from the documentation including any modules that were explicitly mentioned in the code examples / API. Chai already seems very well typed so it should be easy to use that to help with your tests.

[AJamesPhillips]

As we're "breaking things" I also took the step to remove the preceding I from interface names in preparation for passing dtslint rules. There's also a note at the top regarding backwards compatibility and how to migrate.

[AJamesPhillips]

@jasonswearingen I have submitted it as a PR. Will do further testing next week but running over the tranche of tests now included (mostly copied directly from the docs) has made me more confident this will be doing what we'd expect it to.

[fabm22]

I know this might seem totally unfounded depending on the actual "source" package structure, but would it not be possible to have some kinda of release cycle for the typescript definitions near or in sync with the release cycle of Hapi.js ?