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(http): add iana methods to the http namespace #4320

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

halvardssm
Copy link
Contributor

Adds IANA Methods as a follow-up from #4304

@github-actions github-actions bot added the http label Feb 12, 2024
@kt3k kt3k added the feedback welcome We want community's feedback on this issue or PR label Feb 13, 2024
@Leokuma
Copy link

Leokuma commented Feb 13, 2024

I'm -1 because the method can actually be any string. So this PR looks more like a documentation than something worth being in code, especially splitting by norms. I think the name "Rfc9110" should not be in the implementation, but in comments at most. To users, it shouldn't matter where a standard is defined, just like it doesn't matter where and when fetch() was specified.

@iuioiua
Copy link
Collaborator

iuioiua commented Feb 15, 2024

I'm -1 on this change also. We previously had a methods implementation in std/HTTP but removed it because it was barely used (see #3792). Even if I were for the addition, I'd much prefer the style previously used (see https://github.com/denoland/deno_std/blob/207f5cad67150e2bf0b8a169e33f7130b4ebb9c3/http/unstable_method.ts).

@halvardssm
Copy link
Contributor Author

To address the comments:

@Leokuma As this is not specifically typed or provided anywhere else as an enum or variables, having it available in the standard library would be of benefit. As discussed in #4304, there was disagreement on which standards to follow, so to satisfy all camps I separated them by standards, and still provided a collection at the bottom with all the methods from the standards (see the exported HttpMethod). I agree that it shouldn't matter, where the standard is specified, but I would see the benefit of having the methods like this, rather than not at all. Please let me know if you have a different suggestion as to how we can specify the methods.

@iuioiua Although it was barely used, it was used, and as there is generally a standard around the methods, the standard library should in my opinion be a place to specify this for utility purposes. As mentioned in #4304, there was not a consensus on the methods to include, and therefore I decided to split it like this. By using this style, there is also more information regarding the origin of the method, which rfc it is related to, and where to find more information regarding its usage.

To summarise the reason for this contribution (all of the points are my own opinions):

The methods are generally standardized, but allow for any string to be used. By providing the common standards as well as a collected export, we will cater to the different usages (most). It is in my opinion better to provide this, even if it is not heavily used, than not at all.

Feedback on how we can improve on my PR is appreciated!

@Leokuma
Copy link

Leokuma commented Feb 15, 2024

Can you provide an example use case just so we can see how you envision this being used? I'm guessing it would be used to type routers or something?

@halvardssm
Copy link
Contributor Author

@Leokuma At the top of my head, I can think of two use cases:

In fetch

fetch("example.com",{method: HttpMethod.Post})

In deno serve, router or lambda

Deno.serve(({method})=>{
  if(method === HttpMethod.Get){
    // do something
  }
})

@iuioiua
Copy link
Collaborator

iuioiua commented Feb 16, 2024

Why not just use "GET"? Genuine question. One argument would be correctness. But I have yet to see any examples of this being a problem.

@halvardssm
Copy link
Contributor Author

I get what you mean, so in practice, nothing is stopping anyone from using "GET". It is just an accessible helper const/type for those that need it, or want extra typings support. It can also be used for typing a switch, or restrict input for helper functions etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback welcome We want community's feedback on this issue or PR http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants