Skip to content

Conversation

@targos
Copy link
Member

@targos targos commented Dec 8, 2019

I tried to use the new feature that TypeScript added for assertions functions to fix the type of abortIf but couldn't find a way. abortIf is implemented in the opposite way (throws if the condition is truthy) than usual assertions.

Ref: https://devblogs.microsoft.com/typescript/announcing-typescript-3-7/#assertion-functions

@targos
Copy link
Member Author

targos commented Dec 11, 2019

/cc @thetutlage

@targos
Copy link
Member Author

targos commented Dec 17, 2019

Ping. Even if we don't find a solution for abortIf, I think this change is important to get in.

@thetutlage
Copy link
Member

Yeah, we need inverse of asserts <val>. Maybe we can also add another function abortUnless?

@targos
Copy link
Member Author

targos commented Dec 19, 2019

Yeah, we need inverse of asserts <val>. Maybe we can also add another function abortUnless?

Do you want me to add that to this PR?

@thetutlage
Copy link
Member

Yeah, let's get them out together

This method is the inverse of abortIf. It allows to use type assertions
to make TypeScript narrow types if the function threw.
@targos
Copy link
Member Author

targos commented Dec 19, 2019

Done.

test('abortUnless: abort request when condition is falsy', async (assert) => {
const server = createServer((req, res) => {
const config = fakeConfig()
const response: Response = new Response(req, res, config)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See microsoft/TypeScript#33622

It is necessary here to explicitly declare the type of Response. In practice in Adonis, the type is already defined in the context so the user doesn't have to do anything.

Example with the change, where TS correctly determined that the code after the call is unreachable:
image

@thetutlage thetutlage merged commit 96049b2 into adonisjs:develop Dec 23, 2019
@thetutlage
Copy link
Member

Thanks 😄

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.

2 participants