Skip to content

Conversation

ibodev1
Copy link
Contributor

@ibodev1 ibodev1 commented Apr 18, 2023

No description provided.

@yusukebe
Copy link
Member

HI @ibodev1 !

Thanks for creating the PR.

It looks good, but I'd like to reconsider the API, particularly for functions like serve, though it may not be a big change. Therefore, I will leave this PR unmerged for now.

@ibodev1
Copy link
Contributor Author

ibodev1 commented Apr 22, 2023

Hi @yusukebe.

I like your work very much. I like using nodejs very much, I recently started using deno and the hono library is really useful and fast. I have a callback function habit from using node js and express. I had this deficiency in hono and I wanted to open PR. I hope it will help you.

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.

@ibodev1

Sorry for delay. I like this feature and the API. So, I want to merge it.
I've leaved one comment. Check it!

src/server.ts Outdated
const server = createAdaptorServer(options);
const serverInfo = server.address() as AddressInfo;
const port = serverInfo?.port ?? options.port;
server.listen(options.port || 3000, options.hostname || '0.0.0.0', () => {
Copy link
Member

Choose a reason for hiding this comment

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

The first argument of server.listen should be port.

@ibodev1
Copy link
Contributor Author

ibodev1 commented Apr 25, 2023

hi @yusukebe ,

No problem mate. i am very glad that you want to merge. is there anything i need to do? i am very new to pull request. and my english is terrible. if there is anything i need to do, i will help if you tell me in detail. thank you.

I would like to make a suggestion. if possible a callback function can be added for other adapters.

@ibodev1
Copy link
Contributor Author

ibodev1 commented Apr 25, 2023

As far as I understand from the comment, I made such an update.

@yusukebe
Copy link
Member

Hi @ibodev1 !

Thanks for updating.

I think it would be more convenient to pass the AddressInfo rather than passing the port to the listeningListener callback, what do you think?

Then we can write the following.

serve(app, (info) => { // info is AddressInfo
  console.log(`Running on ${info.address}:${info.port}`)
})

@ibodev1
Copy link
Contributor Author

ibodev1 commented Apr 26, 2023

I made the updates you asked for and committed it. I hope it worked for you. I made an update in the readme file about how the listener is used.

@ibodev1
Copy link
Contributor Author

ibodev1 commented Apr 26, 2023

I edited the readme file under the heading "Usage" instead of opening a separate heading. it is more readable this way.

@yusukebe
Copy link
Member

Hi @ibodev1 !

Perfect! I'll merge it now. Thanks for your contribution!

@yusukebe yusukebe merged commit da05ac4 into honojs:main Apr 27, 2023
@ibodev1
Copy link
Contributor Author

ibodev1 commented Apr 27, 2023

you are welcome. see you in other projects :)

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