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

feat: Returns the parent's app from route(path, subApp). #948

Closed
wants to merge 1 commit into from

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Mar 3, 2023

#947

As for route(), in general, I think the expected type depends on the arguments given.

If no subApp is given, then of course an app with path as basePath should be returned.

const subApp = app.route('/api')

However, if a subApp is given, the app that has it in the basePath is usually not necessary.

const probablyNeverBeUsed = app.route('/api', subApp)

If so, it would be more convenient to return this when a subApp is specified. This PR includes such a change.

Is this a good API?

However, it is generally not a good API to have the return type change depending on the argument. I think we can avoid this confusion by renaming the single argument route() as basePath() and make the return value intuitive as well.
main...usualoma:hono:feat/basePath-method

Other options

I don't think the current behavior of main branch is so bad, so I think there is an option not to change it.

@yusukebe
What do you think?

@yusukebe
Copy link
Member

yusukebe commented Mar 3, 2023

Hi @usualoma !

I've checked and I'm considering it. Give me a minute. Thanks!

@yusukebe
Copy link
Member

yusukebe commented Mar 5, 2023

Hi @usualoma !

Thank you for the PR.

This looks great to me. It was bad that we can not "chain" because app.route() returning "subApp". It is better to returns this.

I agree with creating a basePath(). It is easier for users to understand.


If we are merging, please direct it to the next branch. This will be used for the next minor version "v3.1".

@yusukebe
Copy link
Member

This PR is not needed now. Closing.

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