deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

feat(http): add iana methods to the http namespace

Open halvardssm opened this issue 2 years ago • 7 comments

Adds IANA Methods as a follow-up from #4304

halvardssm avatar Feb 12 '24 15:02 halvardssm

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.

Leokuma avatar Feb 13 '24 19:02 Leokuma

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).

iuioiua avatar Feb 15 '24 00:02 iuioiua

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!

halvardssm avatar Feb 15 '24 09:02 halvardssm

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?

Leokuma avatar Feb 15 '24 14:02 Leokuma

@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
  }
})

halvardssm avatar Feb 15 '24 23:02 halvardssm

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.

iuioiua avatar Feb 16 '24 23:02 iuioiua

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.

halvardssm avatar Feb 17 '24 00:02 halvardssm

Can you clarify what tests you envision? Since it's a const, it's not much to test in regards to functionality and I am not sure testing the values gives much either, no?

halvardssm avatar Aug 03 '24 10:08 halvardssm

Tests can suffice being simple/light. It's just to ensure the plumbing works and will continue to work in the future. PTAL at the tests in http/status_test.ts.

iuioiua avatar Aug 05 '24 05:08 iuioiua

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.39%. Comparing base (fdadd53) to head (19ae628).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4320      +/-   ##
==========================================
+ Coverage   96.38%   96.39%   +0.01%     
==========================================
  Files         466      467       +1     
  Lines       37588    37712     +124     
  Branches     5542     5542              
==========================================
+ Hits        36228    36352     +124     
  Misses       1318     1318              
  Partials       42       42              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 05 '24 16:08 codecov[bot]

@iuioiua Done! Let me know if there is anything else!

halvardssm avatar Aug 05 '24 16:08 halvardssm