Skip to content

feat(http/unstable): methods module#4320

Merged
iuioiua merged 9 commits intodenoland:mainfrom
halvardssm:feat/add-iana-methods
Aug 6, 2024
Merged

feat(http/unstable): methods module#4320
iuioiua merged 9 commits intodenoland:mainfrom
halvardssm:feat/add-iana-methods

Conversation

@halvardssm
Copy link
Copy Markdown
Contributor

Adds IANA Methods as a follow-up from #4304

@halvardssm halvardssm requested a review from kt3k as a code owner February 12, 2024 15:48
@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
Copy Markdown

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Yoshiya and I read over this again, and are now thinking something like this is worth having.

Can you please add some simple tests?

Comment thread http/method.ts Outdated
Comment thread http/method.ts Outdated
@halvardssm
Copy link
Copy Markdown
Contributor Author

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?

@iuioiua
Copy link
Copy Markdown
Contributor

iuioiua commented Aug 5, 2024

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 iuioiua changed the title feat(http): add iana methods to the http namespace feat(http): @std/http/methods Aug 5, 2024
halvardssm and others added 2 commits August 5, 2024 18:07
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kt3k kt3k changed the title feat(http): @std/http/methods feat(http/unstable): @std/http/methods Aug 5, 2024
@halvardssm halvardssm requested a review from iuioiua August 5, 2024 16:32
@halvardssm
Copy link
Copy Markdown
Contributor Author

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

@iuioiua iuioiua changed the title feat(http/unstable): @std/http/methods feat(http/unstable): methods module Aug 6, 2024
@iuioiua iuioiua changed the title feat(http/unstable): methods module feat(http/unstable): methods module Aug 6, 2024
Copy link
Copy Markdown
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

@iuioiua iuioiua enabled auto-merge (squash) August 6, 2024 09:22
@iuioiua iuioiua merged commit a9f3558 into denoland:main Aug 6, 2024
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.

4 participants