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: extends send result to provide ability of custom handling #80

Merged
merged 3 commits into from
Jul 12, 2024

Conversation

climba03003
Copy link
Member

@climba03003 climba03003 commented Jul 12, 2024

I would like to extends the result to provide ability of custom handling. Related to fastify/fastify-static#454

I don't like the hooks or event approach because it is more explicit and requires the users to do more fundamental works.
For example,

  1. providing the directory listing no longer requires the users to handling the trailing slash case.

Benefits of the current approach,

  1. Do not needs to handle the async-await, callback or sync function.
  2. Ensure consistence result.

Drawback of the current approach,

  1. We create a stream no matter the users will be use it or not.

Checklist

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

I also prefer this

We create a stream no matter the users will be use it or not.

I mean the user should expect to have a stream after a successful call to send in my opinion

@climba03003
Copy link
Member Author

climba03003 commented Jul 12, 2024

I do see some case that will not be able to cover.
Currently, @fastify/static is depending on the case before the trailing slash handling.
https://github.com/fastify/fastify-static/blob/next/test/dir-list.test.js#L146-L172
Since, 403 is not unique status code fired inside sendRedirect.
It is hard to support the directory listing in fastify.

For example,

send/lib/send.js

Lines 592 to 594 in efbaed1

if (hasTrailingSlash(options.path)) {
return sendError(403)
}

Edit: Nevermind, I manage to handle the problem on @fastify/static.

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

@mcollina mcollina merged commit d28f7d3 into master Jul 12, 2024
16 checks passed
@mcollina mcollina deleted the extends-result branch July 12, 2024 09:54
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.

3 participants