Skip to content

feat: extends send result to provide ability of custom handling#80

Merged
mcollina merged 3 commits intomasterfrom
extends-result
Jul 12, 2024
Merged

feat: extends send result to provide ability of custom handling#80
mcollina merged 3 commits intomasterfrom
extends-result

Conversation

@climba03003
Copy link
Copy Markdown
Member

@climba03003 climba03003 commented Jul 12, 2024 •
edited
Loading

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

@climba03003 climba03003 mentioned this pull request Jul 12, 2024
Merged
4 tasks
gurgunday
gurgunday approved these changes Jul 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@gurgunday gurgunday left a comment •
edited
Loading

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
Copy Markdown
Member Author

climba03003 commented Jul 12, 2024 •
edited
Loading

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.

mcollina
mcollina approved these changes Jul 12, 2024
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@mcollina mcollina mcollina approved these changes

@gurgunday gurgunday gurgunday approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@climba03003 @mcollina @gurgunday