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

feat: support async constraint #4323

Merged
merged 1 commit into from Oct 10, 2022
Merged

feat: support async constraint #4323

merged 1 commit into from Oct 10, 2022

Conversation

climba03003
Copy link
Member

@climba03003 climba03003 commented Oct 6, 2022

Add support for async constraints

Checklist

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 6, 2022

Does hasRoute need also some love?

@climba03003
Copy link
Member Author

climba03003 commented Oct 6, 2022

Does hasRoute need also some love?

There is no API changes in hasRoute or find-my-way.find

@climba03003
Copy link
Member Author

CI failure due to Github Actions outage
https://www.githubstatus.com/incidents/gq1x0j8bv67v

I will re-run the CI after Github fix it.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 6, 2022

Github Actions run again properly
Restarted the failed CI

Code coverage is low

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -732,6 +732,56 @@ fastify.route({
})
```

#### Asynchronous Custom Constraints
Copy link
Member Author

Choose a reason for hiding this comment

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

@Fdawgs Do you have time to helps me check the document changes?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry @climba03003, missed this. Will take a look next week.

fastify.js Outdated Show resolved Hide resolved
fastify.js Show resolved Hide resolved
fastify.js Outdated Show resolved Hide resolved
'Content-Type': 'application/json',
'Content-Length': body.length
})
res.end(body)
Copy link
Member

Choose a reason for hiding this comment

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

Did you evaluate res.destroy instead?

The only difference is whether or not emit an error from the request object. Can't recall any other differences

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite understand what you means.

Copy link
Member

Choose a reason for hiding this comment

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

I needed to refresh this

const { fastify } = require('fastify')
const app = fastify()

app.get('/destroy', async (request, reply) => {
  reply.hijack()
  reply.raw.destroy('hello world')
})

app.get('/end', async (request, reply) => {
  reply.hijack()
  reply.raw.end('hello world')
})

app.inject('/end', check)
  .then(() => app.inject('/destroy', check))

function check (err, res) {
  console.log(err)
  console.log(res?.statusCode)
  console.log(res?.payload)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

.destory cannot and will not send any payload to the client. Instead, it close the socket immediately.
.end return a proper error message to the client and they can deal with it other than just wait for browser socket timeout.

docs/Reference/Routes.md Outdated Show resolved Hide resolved
docs/Reference/Routes.md Outdated Show resolved Hide resolved
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants