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

Correct Hapi route cache configuration typing #16416

Closed
wants to merge 1 commit into from

Conversation

theefer
Copy link

@theefer theefer commented May 9, 2017

Update types to include all options (as optional) and expiresIn/At as mutually exclusive

  • 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://hapijs.com/api#route-options
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dslint/dt.json" }.

…ptional) and expiresIn/At as mutually exclusive
@dt-bot
Copy link
Member

dt-bot commented May 9, 2017

types/hapi/index.d.ts

to author (@jasonswearingen). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@jasonswearingen
Copy link
Contributor

👍 looks good!

AJamesPhillips added a commit to AJamesPhillips/DefinitelyTyped that referenced this pull request May 11, 2017
@AJamesPhillips
Copy link
Contributor

@theefer I saw your typings and got excited again by the prospect of enforcing what you're hoping to enforce there. But as you can see from the tests it doesn't do what you're hoping for unfortunately. For example you can still define both:

type OneOrTheOther = ({
    expiresIn: number;
} | {
    expiresAt: string;
});

const unsupportedOnlyOne: OneOrTheOther = {
    expiresIn: 5000,
    // expecting an error about having both attributes defined but no error as
    // this object correctly fulfils the type requirements of `OneOrTheOther`.
    // I suspect it might have something to do with Liskov Substitution Principle
    // but not sure http://stackoverflow.com/a/23305134/539490
    expiresAt: '22:44',
}

In fact the compiler is happy with one with a wrong type as long as the other is present and correct:

const badOnlyOne: OneOrTheOther = {
    expiresIn: '5000',  // expecting error but none
    expiresAt: '22:44',
}

In fact with the | {} part it's worse as it accepts complete rubbish then :(

type OneOrTheOtherOrNeither = ({
    expiresIn: number;
} | {
    expiresAt: string;
} | {});

const reallyBadOnlyOne: OneOrTheOtherOrNeither = {
    expiresIn: 'invalid',
    expiresAt: { invalid: '' },
}

AJamesPhillips added a commit to AJamesPhillips/DefinitelyTyped that referenced this pull request May 11, 2017
@AJamesPhillips
Copy link
Contributor

Exclusive unions would be useful for this: microsoft/TypeScript#14094

@AJamesPhillips
Copy link
Contributor

AJamesPhillips commented May 12, 2017

Right, we need this as it works as expected:

type OneOrTheOther = ({
    expiresIn: number;
    expiresAt?: undefined;
} | {
    expiresIn?: undefined;
    expiresAt: string;
} | {
    expiresIn?: undefined;
    expiresAt?: undefined;
});

(Obviously now the typings still have minimal gain for quite a lot of complexity but at least it works as we'd like it to)

AJamesPhillips added a commit to AJamesPhillips/DefinitelyTyped that referenced this pull request May 12, 2017
@theefer
Copy link
Author

theefer commented May 12, 2017

Thanks for correcting my mistakes, sorry it was my first attempt at an official d.ts contribution and I'm still learning and figuring out how to test these things properly. I saw that you included for fixed version in your branch, so shall I just close this PR?

@AJamesPhillips
Copy link
Contributor

AJamesPhillips commented May 12, 2017

There's absolutely no need for apology. You saw (part of) the learning trip I took to get to the current solution... I should be thanking you... so thank you! :)

The version of v15/index.d.ts in my commit is what your index.d.ts is at the moment. I'm happy for you to close this PR and I'll update the v15/index.d.ts in mine. Or you can update yours and have this PR merged in. Whatever you wish. If you want these changes asap I would recommend you update your index.d.ts and then we'll ping a maintainer to merge this PR as #16065 is taking a while.

@jasonswearingen
Copy link
Contributor

Ahh, I too was excited... perhaps too excided. I thought that type-union enforcement was a new feature of 2.3, but I guess not :(. Thanks for the actual in-ide check @AJamesPhillips

@aozgaa
Copy link
Contributor

aozgaa commented May 16, 2017

merged #16065

@aozgaa aozgaa closed this May 16, 2017
@AJamesPhillips
Copy link
Contributor

@theefer feel free to update the v15 typings with these changes if you wish and submit another PR 👍 Thank you very much for this though. The changes are in the reworked hapi.js 16.1 typings thanks to you :)

@theefer
Copy link
Author

theefer commented May 18, 2017

No worries, thanks for the update!

@theefer theefer deleted the hapi branch May 18, 2017 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants