Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Oct 31, 2024

At runtime headers may not be an instanceof Headers.

This causes a lot of bugs.

the function may get a js object as an argument.

Instead of looping directly through headers, I get the entries then loop through them.

Mohamed Gad and others added 2 commits October 31, 2024 10:23
At runtime headers may not be an `instanceof` Headers. 

This causes a lot of bugs. 

the function may get a js object as an argument. 

Instead of looping directly through `headers`, I get the entries then loop through them.
@yusukebe
Copy link
Member

Hi @mjad218

Thank you for the PR. Can you run yarn lint && yarn format to format the code?

@ghost
Copy link
Author

ghost commented Oct 31, 2024

Sure, will push a commit soon

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

@mjad218

Thanks! Merging.

@yusukebe yusukebe merged commit 909e21e into honojs:main Oct 31, 2024
@usualoma
Copy link
Member

Hi, @mjad218, Thanks for creating the pull request.
cc: @yusukebe

I'm sorry, but I didn't understand the need for this change. At least inside "node-server," I don't think anything other than "Headers" is passed as an argument. Does this mean it is used by directly importing it from the application?

I'd like to know the specific use case.

Also, when merging, we may need to add test code.

@ghost
Copy link
Author

ghost commented Oct 31, 2024

@usualoma
I am using a Hone NestjsAdapter. The adapter actually does not call buildOutgoingHttpHeaders though.

@ghost
Copy link
Author

ghost commented Oct 31, 2024

@ghost
Copy link
Author

ghost commented Oct 31, 2024

I believe what you are saying is correct given that it is called with valid arguments and you validate the headers object.

I reviewed its usage in the package here, I noticed whenever it's used, it has an if (headers instanceof Headers) condition before it. line 49

There was one place in the package that did not have this condition. line 90

Moving the validation inside the function itself is good; at runtime, you don't know what type of arguments it will get. (TS does not guarantee runtime checks)

@usualoma
Copy link
Member

@mjad218 Thanks for the reply!

In the following line, we are referencing the headers property of the Response object so we can treat it as a Headers object without checking.

const resHeaderRecord: OutgoingHttpHeaders = buildOutgoingHttpHeaders(res.headers)

https://developer.mozilla.org/en-US/docs/Web/API/Response/headers

It is possible to break this convention in an application, but that is its responsibility, and it is not something that the node-server should handle.

Also, I don't think any Hono users have written code that returns a Request object that returns something other than a Headers object here.

I think that I don't need these changes.

@ghost
Copy link
Author

ghost commented Oct 31, 2024

@usualoma I am using Nest.js and trying to get it working with Hono. And it seems that buildOutgoingHttpHeaders gets called with undefined.

I spent the past few days walking through Hono source code and I like how it's written and structured.

It is possible to break this convention in an application, but that is its responsibility, and it is not something that the node-server should handle.

I think this is the case, Nest.js is not designed to work with Hono. I am just trying to build an adapter for it.

It works best with Express and Fastify.

@usualoma
Copy link
Member

@mjad218
Thanks for the explanation!
Sorry, I finally understood.
If that's the case, I see. We'll have to go with this approach.

@ghost
Copy link
Author

ghost commented Oct 31, 2024

Great, Glad to hear that

@yusukebe
Copy link
Member

@usualoma @mjad218

Thanks!

Also, when merging, we may need to add test code.

I'm sorry, but as @usualoma said, we have to add the test code. I'll add it myself tomorrow and take a test coverage for better quality.

@ghost
Copy link
Author

ghost commented Oct 31, 2024

Sure, that's fine

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.

2 participants