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

Implicit HEAD support #243

Closed
sonnyp opened this issue Apr 8, 2021 · 5 comments
Closed

Implicit HEAD support #243

sonnyp opened this issue Apr 8, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@sonnyp
Copy link
Contributor

sonnyp commented Apr 8, 2021

I'm not entirely sure if it's a bug or a feature request.

Describe the bug

GET routes do not have implicit support for HEAD requests.

Most (all?) http frameworks out there have implicit HEAD requests for GET routes, they will simply ignore the response body.
Also, app.head is not documented which makes it difficult to workaround.

My opinion is that if a GET route doesn't have an associated HEAD route, support should be implicit.

To Reproduce

import {App} from '@tinyhttp/app';

const app = new App();

function ping(req, res) {
  res.statusCode = 204;
  res.end();
}
// app.head('/ping', ping); // workaround
app.get('/ping', ping);

app.listen(3000);
curl -v -X HEAD http://localhost:3000/ping # -> 404 not found

Expected behavior

curl -v -X HEAD http://localhost:3000/ping # -> 204

Versions

  • node: 14
  • @tinyhttp/app: 1.2.20
@sonnyp sonnyp added the bug Something isn't working label Apr 8, 2021
@talentlessguy
Copy link
Member

you should use res.send for it to work properly

res.end is a built-in method that doesn't do anything, that's why 404 is returned

@sonnyp
Copy link
Contributor Author

sonnyp commented Apr 8, 2021

It's the same problem with res.send

import {App} from '@tinyhttp/app';

const app = new App();

function ping(req, res) {
  res.status(204).send();
}
// app.head('/ping', ping); // workaround
app.get('/ping', ping);

app.listen(3000);

Of course Node.js res.end does something https://nodejs.org/api/http.html#http_response_end_data_encoding_callback

EDIT: I realize you probably meant doesn't do anything in the context of tinyhttp 👍

@sonnyp
Copy link
Contributor Author

sonnyp commented Apr 8, 2021

looks like res.send does support HEAD https://github.com/talentlessguy/tinyhttp/blob/master/packages/send/src/send.ts#L56

However a app.get route is simply not registered for HEAD. The handler isn't even called.

@talentlessguy
Copy link
Member

@sonnyp hmm probably I need to add a check for req.method in app.handle

@talentlessguy
Copy link
Member

I need to think how to edit find func in a way that I don't have to query the mw twice...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants