Skip to content

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
@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.

4 participants