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: add an utility function to get protocol #306

Closed
1 task done
NozomuIkuta opened this issue Jan 20, 2023 · 7 comments · Fixed by #351
Closed
1 task done

feat: add an utility function to get protocol #306

NozomuIkuta opened this issue Jan 20, 2023 · 7 comments · Fixed by #351
Labels
enhancement New feature or request

Comments

@NozomuIkuta
Copy link
Member

Describe the feature

h3 has some kinds of utility functions to get information about the request (e.g. getMethod()).

It would be also good to have that utility function(s) to get protocol of the request. Currently, we have to install another unjs package, is-https package, to detect HTTPS, or implement similar logic by ourselves.

So, exporting such functionality, like getProtocol or isHTTPS, from h3 would be beneficial for developers in terms of DX.

cf. built-in CORS support (#82) vs. my CORS package


More generally, we might want to create an issue where we can discuss what kind of middlewares and utility functions should be exported from h3, rather than providing them as 3rd-party package.

Additional information

  • Would you be willing to help implement this feature?
@NozomuIkuta NozomuIkuta added the enhancement New feature or request label Jan 23, 2023
@ashutosh887
Copy link

I would like to work on this @NozomuIkuta

@NozomuIkuta
Copy link
Member Author

@ashutosh887

Please feel free to submit a PR 👍

@ashutosh887
Copy link

Sure @NozomuIkuta . Please assign me

@NozomuIkuta
Copy link
Member Author

NozomuIkuta commented Mar 2, 2023

@ashutosh887

I guess we don't use asignees section in this repository.
Since you said you would work on this issue, no one would take it over unless you hand it over.

@ashutosh887
Copy link

Thanks a lot... I would start immediately

@ashutosh887
Copy link

ashutosh887 commented Mar 2, 2023

How would you like the function to be? @NozomuIkuta @pi0
I'm trying to set it up.
Also, I would like to propose an Idea to create a Contributing.md for new comers/contributers to get started and get a general Idea.

@NozomuIkuta
Copy link
Member Author

IMHO, migrating is-https codebase to h3 is an option, like we did for CORS support. For naming, isXYZ would be nice for consistency, because we already have isPreflightRequest and isCorsOriginAllowed.

Other related functions (e.g. getProtocol) could utilize ufo package under the hood.

You might want to submit a PR anyway based on your ideas and start a discussion in PR thread.

Whichever, @pi0 have the right to determine the final spec as the author of h3 (according to unjs governance policy).


As for contribution, here are summary for you at the moment:

  • Use Node.js LTS version
  • Use pnpm by enabling corepack (version is specified in package.json so you don't have to worry about it)
  • You should define utility function(s) in an appropriate place in src/utils directory (either of existing file(s), or new file(s))
  • Make sure to add unit test in an appropriate place in test directory (otherwise, they wouldn't merge your PR)
  • Make sure to add explanation about the new utility function(s) to Utilities section of README
  • It's recommended to add JSDoc comments to the new function(s), so that developers can see API on their editors without googling (if you are not familiar with it, you can ask for help in the PR)

By the way, at the moment, Development section of unjs/template would correspond to CONTRIBUTING.md (chronologically, h3 doesn't have such a section). But, I agree that it's worth adding more information to Development section, so that more and more people like you can contribute with confidence.

You are also welcomed to submit a PR to enhance h3 README, and to unjs/template for future unjs packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants