denoland/std

Clean up plan for `http` module

Closed this issue · 8 comments

kt3k commented

There are 12 public paths in std/http:

  • cookie.ts
  • cookie_map.ts
  • etag.ts
  • file_server.ts
  • http_errors.ts
  • http_status.ts
  • method.ts
  • negotiation.ts
  • server.ts
  • server_sent_event.ts
  • user_agent.ts
  • util.ts

I'd suggest we should stabilize the following in first pass:

  • cookie.ts
  • etag.ts
  • file_server.ts
  • http_status.ts (The path might be renamed #3646)
  • negotiation.ts
  • user_agent.ts

and I'd suggest we move the below to unstable/ sub directory of std/http

  • cookie_map.ts (Looks overly complex. Not sure signed cookie is best practice today. KeyRing abstraction might be confusing)
  • http_errors.ts (Not sure error status code as Error object is the general enough abstraction)
  • method.ts (Our mothod list deviates from IANA's method registry, instead aligned to http.METHODS of node.js. Not sure this is correct)
  • server.ts (Server is confusing as now CLI has Deno.Server. Also the usage of this is niche now as CLI has Deno.serve).
  • server_sent_event.ts (The issue #3645 looks concerning)
  • util.ts (Not sure it's worth exposing this (createCommonResponse) as std API)
bartlomieju commented

I'd suggest we should stabilize the following in first pass:

This makes sense to me 👍 these seem to be very stable

and I'd suggest we move the below to unstable/ sub directory of std/http

  • cookie_map.ts (Looks overly complex. Not sure signed cookie is best practice today. KeyRing abstraction might be confusing)

Agreed, let's not stabilize this one.

  • http_errors.ts (Not sure error status code as Error object is the general enough abstraction)

This seems useful and I would err on stabilizing it.

  • method.ts (Our mothod list deviates from IANA's method registry, instead aligned to http.METHODS of node.js. Not sure this is correct)

I think we should align with IANA here

  • server.ts (Server is confusing as now CLI has Deno.Server. Also the usage of this is niche now as CLI has Deno.serve).

That's a though one; maybe we should deprecate and remove it once we provide feature parity in Deno.serve API?

Can we change the class to expose the body and fire off requested events?

  • util.ts (Not sure it's worth exposing this (createCommonResponse) as std API)

We should move this method into file_server.ts and delete the module

  • 👍1
kt3k commented

Thanks for the feedbacks!

  • http_errors.ts (Not sure error status code as Error object is the general enough abstraction)

This seems useful and I would err on stabilizing it.

There's also similar feedback from Asher. Let's keep discussing this in an independent issue.

  • method.ts (Our mothod list deviates from IANA's method registry, instead aligned to http.METHODS of node.js. Not sure this is correct)

I think we should align with IANA here

👍

  • server.ts (Server is confusing as now CLI has Deno.Server. Also the usage of this is niche now as CLI has Deno.serve).

That's a though one; maybe we should deprecate and remove it once we provide feature parity in Deno.serve API?

Sounds reasonable to me.

  • util.ts (Not sure it's worth exposing this (createCommonResponse) as std API)

We should move this method into file_server.ts and delete the module

I agree with this idea.

iuioiua commented

I think we should re-open this issue. I agree with the following with pretty much the same opinions:

  • Remove cookie_map.ts
  • Stabilise errors.ts
  • Align method.ts with IANA

Which features are we waiting on for http/server.ts to be on par with Deno.serve()?

kitsonk commented

@kt3k

  • cookie_map.ts (Looks overly complex. Not sure signed cookie is best practice today. KeyRing abstraction might be confusing)

Why would you not consider it best practice today? It isn't encrypting the cookie, it is signing the cookie to prevent tampering.

@iuioiua

[ ] Remove cookie_map.ts

Why?

iuioiua commented

@kitsonk, we considered removing cookie_map.ts because it overlaps with cookie.ts, which uses more straightforward pure functions. According to analytics, it also has nearly no usage, probably for that reason. Are there any advantages to using cookie_map.ts over cookie.ts?

kitsonk commented

Secure cookie maps offers transparent cookie security to help ensure that client side cookies are not tampered with. It was a foundational API of oak based on similar concepts around cookie management in Koa. It was contributed to std. It's relative low usage is in part due to relative low adoption of more recent versions of oak where it was contributed to std and has been in std far shorter time period than cookie, less than a year.

The insecure version was requested to be added to keep API parity.

If there is such a keen desire to get rid of it, then it should have never been accepted in the first place.

iuioiua commented

Ok. I understand the point of keeping the insecure version, CookieMap, for API parity with SecureCookieMap. Either way, I still think there should only be one cookie implementation.

Also, I'm not opposed to having a signed cookies implementation. My main thought is that it could probably done a little simpler using a functional approach and complimenting the current cookies.ts. I'd have to do a little more research before forming a strong opinion.

kt3k commented

@kitsonk

Why would you not consider it best practice today? It isn't encrypting the cookie, it is signing the cookie to prevent tampering.

The way to sign it isn't standardized (or is it?). Application or framework can sign it in their own ways. I'm not confident providing it from deno_std is a good idea.

There's also alternative and standardized way to sign data such as JWT. I'm also not sure these methods could compete with signed cookie at this point.

BTW cookie_map is not deprecated or removed. They are just marked unstable to make it possible for us to explore it more extended time (Non-unstable modules will be released as v1.0 near future (probably 2024 Q1)).