Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@types/hapi - v17 #22107

Merged
merged 93 commits into from Dec 20, 2017

Conversation

Projects
None yet
8 participants
@rafaelsouzaf
Copy link
Contributor

rafaelsouzaf commented Dec 11, 2017

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/hapijs/hapi/blob/master/API.md
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

rafaelsouzaf added some commits Nov 22, 2017

Replaced "any" type to "RouteCompressionEncoderSettings" type to bett…
…er understanding because is shared with other interface.
* * 'disconnect' - emitted when a request errors or aborts unexpectedly.
* [See docs](https://github.com/hapijs/hapi/blob/master/API.md#-requestevents)
*/
on(criteria: RequestEventType, listener: Function): void;

This comment has been minimized.

@SimonSchick

SimonSchick Dec 16, 2017

Contributor

You can use a mapped type to declare all on/once methods on this interface.

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

@SimonSchick can you give me more info please? I read about Mapped Type (link-1, link-2) but I can't see how to use in this case. We are overriding methods (on/once) from Podium interface but with some differents parameters. How can I use Mapped Type in this case please? Thanks for the feedback!

*/
export interface ServerMethod {
// (args: any[], flags: {ttl?: number}): any;
(...args: any[]): any; // TODO it needs review. Look the doc: the first param is a ...args and the second is "flags". Typescript supports ...args only in the last param

This comment has been minimized.

@SimonSchick

SimonSchick Dec 16, 2017

Contributor

This should probably be a type, not an interface?

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

Commit b923c71. Thanks!

/**
* TODO: Missing documentation. Exist only in examples and test files.
*/
config?: any;

This comment has been minimized.

@SimonSchick

SimonSchick Dec 16, 2017

Contributor

Probably want to make this a consumer extensible interface.

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

@SimonSchick Yes, I understand. The problem is that I don't have any docs or a good example to make a interface of this config object. For that I preferred to use an "any" for now. But I am open for new ideas of course :-)

This comment has been minimized.

@SimonSchick

SimonSchick Dec 17, 2017

Contributor

I'd just make an empty interface and export it.

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

Commit 065b070

/** password used for 'iron' encoding (must be at least 32 characters long). */
password?: string;
/** options for 'iron' encoding. Defaults to require('iron').defaults. */
iron?: object; // TODO make iron definitions and getting typing from iron. Needs review!

This comment has been minimized.

@SimonSchick

SimonSchick Dec 16, 2017

Contributor
declare module 'iron' {
    export interface ISomething {
        saltBits: number;
        algorithm: string;
        iterations: number;
        minPasswordlength: number;
    }

    export interface ISealOptions {
        encryption: ISomething;
        integrity: ISomething;

        ttl: number;
        timestampSkewSec: number;
        localtimeOffsetMsec: number;
    }

    export const defaults: ISealOptions;

    export const algorithms: {
        'aes-128-ctr': {
            keyBits: number;
            ivBits: number;
        };
        'aes-256-cbc': {
            keyBits: number;
            ivBits: number;
        };
        'sha256': {
            keyBits: number;
        };
    };

    export const macFormatVersion: string;
    export const macPrefix: string;

    export interface IGenerateKeyOptions extends Pick<ISomething, 'algorithm' | 'iterations' | 'minPasswordlength'> {
        saltBits?: number;
        salt?: string;
        iv?: string;
    }

    export interface IKey {
        key: Buffer;
        salt: string;
        iv: string;
    }

    export function generateKey (password: string, options: IGenerateKeyOptions): Promise<IKey>;
    export function encrypt (password: string, options: IGenerateKeyOptions, data: string): Promise<{ data: Buffer, key: IKey }>;
    export function decrypt (password: string, options: IGenerateKeyOptions, data: string): Promise<Buffer>;

    export interface IHMacResult {
        digest: string;
        salt: string;
    }

    export function hmacWithPassword(password: string, options: IGenerateKeyOptions, data: string): Promise<IHMacResult>;

    export function seal (obj: object, password: string, options: ISealOptions): Promise<string>;
    export function unseal (data: string, password: string, options: ISealOptions): Promise<object>;
}

no warranties for correctness.

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

Nice! Thanks @SimonSchick. Is it your code or did you get it from another place?

If it's new, and looks good comparing to Iron js code, is not better to make a new PR creating the /types/iron ? Or should I put this code inside our hapi definitions?

This comment has been minimized.

@SimonSchick

SimonSchick Dec 17, 2017

Contributor

I just figured I would post it here, I didn't have time to PR these typings yet, feel free to PR them yourself.

This comment has been minimized.

@SimonSchick

SimonSchick Dec 17, 2017

Contributor

And yes, I wrote those typings.

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

Commit 62ae76a. Thanks @SimonSchick

cache.set('norway', 'oslo', 10 * 1000, null);
const value = cache.get('norway', null);

// TODO Not working. It needs review.

This comment has been minimized.

@SimonSchick

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

Sorry. It was an old comment. @BorntraegerMarc told me to take it out but I forgot. Commit 3fe936d

server.start();

const res = server.inject('/');
console.log(res.result); // 'Success!' // TODO It's not working

This comment has been minimized.

@SimonSchick

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

The same here. I forgot to remove this comment. Commit 3fe936d

(Object | Object[]) |
// a toolkit signal:
// a toolkit method response:
any; // TODO need review

This comment has been minimized.

@SimonSchick

SimonSchick Dec 16, 2017

Contributor

Probably remove any in favour of stricter types?

This comment has been minimized.

@rafaelsouzaf

rafaelsouzaf Dec 17, 2017

Author Contributor

Commit 244e1ee. Replaced "any" with other types after reviewing the documents again.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Dec 17, 2017

🔔 @SimonSchick - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@SimonSchick

This comment has been minimized.

Copy link
Contributor

SimonSchick commented on types/hapi/definitions/util/iron.d.ts in 62ae76a Dec 17, 2017

This should be in a separate package, not util!

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf replied Dec 17, 2017

Hummm, /definitions/iron ?

/definitions/util
/definitions/server
/definitions/...
/definitions/iron

This comment has been minimized.

Copy link
Contributor

SimonSchick replied Dec 17, 2017

No, this should be it's own package in /types directory, so consumers can install them.

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf replied Dec 17, 2017

Ok. In this case I will need to submit a new PR only for iron, right? No problem at all, but it will take more time to approve this current PR :-(

This comment has been minimized.

Copy link
Contributor

SimonSchick replied Dec 17, 2017

I don't think the PR would be accepted with 3rd party typings in util, you'll have to do it regardless.

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf replied Dec 18, 2017

@SimonSchick I understand perfectly. No problem. Tomorrow I will submit the new PR!

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf replied Dec 18, 2017

@SimonSchick here the Iron PR 👍

@aozgaa aozgaa merged commit fb68b21 into DefinitelyTyped:master Dec 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SimonSchick

This comment has been minimized.

Copy link
Contributor

SimonSchick commented Dec 20, 2017

@aozgaa this PR was not ready for merge...

@rafaelsouzaf

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf commented Dec 20, 2017

Tomorrow I will start working again with this PR 👍 (or made a new one with updates).

@SimonSchick

This comment has been minimized.

Copy link
Contributor

SimonSchick commented Dec 21, 2017

I just fail to understand why this was merged without any approvals...

@andy-ms @aozgaa

@rafaelsouzaf

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf commented Dec 21, 2017

@SimonSchick Please I need to push some commits. Should I create another PR (from another local branch)?

andy-ms added a commit that referenced this pull request Dec 21, 2017

Revert "Merge pull request #22107 from rafaelsouzaf/master"
This reverts commit fb68b21, reversing
changes made to ae47b3c.

andy-ms added a commit that referenced this pull request Dec 21, 2017

Revert "Merge pull request #22107 from rafaelsouzaf/master" (#22417)
This reverts commit fb68b21, reversing
changes made to ae47b3c.
@SimonSchick

This comment has been minimized.

Copy link
Contributor

SimonSchick commented Dec 21, 2017

@rafaelsouzaf The PR was reverted, you should be able to push the branch again and keep commiting, or open a new PR.

@rafaelsouzaf

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf commented Dec 21, 2017

@SimonSchick now I have a big mess here man :-/
The new commits are not visible here more. Probably because the status is "merged". And I have a huge conflict. I will try to fix it today and create a new PR.

@andy-ms

This comment has been minimized.

Copy link
Contributor

andy-ms commented Dec 21, 2017

@rafaelsouzaf Could you just check out your branch as it was, copy the directory, then checkout master and move it back? I don't think we need to preserve all the separate commits from this branch.

@rafaelsouzaf

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf commented Dec 22, 2017

@andy-ms No problem. I made a new PR here. Thanks!

@SimonSchick Well. I will keep working on this new PR. But in this moment Travis is complaining because the iron library is set with TS 2.6, and hapi v17 with 2.4:
Error: hapi depends on iron but has a lower required TypeScript version.

Can I downgrade the iron to 2.4? Thanks!

@SimonSchick

This comment has been minimized.

Copy link
Contributor

SimonSchick commented Dec 22, 2017

Hapi 17 was released post TS2.6, it makes sense to me to force them all to ts 2.6, but I guess downgrading is fine too.

@rafaelsouzaf

This comment has been minimized.

Copy link
Contributor Author

rafaelsouzaf commented Dec 22, 2017

@SimonSchick I agree and I tried before. But if I use TS 2.6 in Hapi17, I will have similar error with all this types because they are using 2.4 or 2.2:

    /types/joi
    /types/boom
    /types/shot
    /types/mimos
    /types/podium
    /types/catbox
    /types/mime-db

The last one "mime-db" is not a "hapi project". I am not sure about the consequences in changing all this types.

Maybe I can submit a PR to downgrade the iron version now and go ahead with the PR.

@typescript-bot typescript-bot removed this from New Definitions in Pull Request Triage Backlog Dec 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.