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

Remove implicit dependence on static definitions of HTTP methods #19

Open
jaamison opened this issue Aug 19, 2019 · 2 comments
Open

Remove implicit dependence on static definitions of HTTP methods #19

jaamison opened this issue Aug 19, 2019 · 2 comments

Comments

@jaamison
Copy link

Cheers to the good folks who have stepped in to maintain this critical peice of the koa ecosystem. I'm ready to push a PR that cleans up this lib's internal handling of http method definitions, but I want to solicit some community feedback first. (ie this a proposal, not a complaint 👍)

We have some work to do re "allowed"/"standard" http methods. Get ready for some hair-splitting. If you're already bored, stop reading here. Currently, this lib:

  • depends on the (aptly and descriptively named) "methods" external package, which exposes an array of strings representing http methods, which it gets from either node's builtin http module or a hard-coded list if the former is unavailable.
  • accepts a separate consumer-supplied array of allowed http methods passed to the contructor (although this doesn't appear to be documented)
  • maintains a per-layer list of methods that are actually used by declared routes

Let's ignore for now the cliche topic of minimizing external dependencies and the fact that many take the number of deps into consideration when evaluating a project's integrity/stability and suitability for use in their own projects.

Open Questions:

  • Do we want this lib to implicitly depend on a nodejs execution environment? And on a mutable global at that?

  • Do we want this lib to be restricted to standards-compliant http? Are we comfortable with the fact that there are esoteric use cases for which Koa, due its generality, works just fine but this lib will not?

  • Are we comfortable, from a documentation and API contract perspective, with the fact that dynamic details of the runtime environment are able to significantly alter the top-level API that this lib exposes? (ie the method signature of Router class instances)

  • Are we misleading consumers and violating the principle of least surprise by having the constructor accept a config/options arg of allowed http methods which is then not respected (consistently)?

  • Are we similarly violating the principle of least surprise with the way that .all() routes are defined? What does a consumer expect to happen when they bind a route definition to "all" http methods? (I doubt they'd expect it to match MKCALENDAR but not a hypothetical custom method they explicitly passed as a constructor arg)

Observations:

  • this lib will instantly throw in a runtime where the commonjs module http isn't resolvable

  • an exotic, non-standard or custom http module has the potential to make this lib's behavior undefined

  • manual runtime mutation of node's builtin http.METHODS global constant has the potential to make this lib's behavior undefined

  • the existence of any value whatsoever for http.METHODS will prevent the well-known defaults (that are hard-coded) from being referenced at all

  • It is possible for the values returned by a router's allowedMethods() to be inconsistent with requests that the router will actually route

I propose that we:

  • ditch the methods dependency, along with all assumptions that a builtin http module is present, that it is in use, and that it constitutes an authoratative enum of all allowed http methods

  • decouple this lib from any particular protocol-level construct (such as the HTTP spec itself and its list of standard supported methods) in favor of a more abstract, protocol-agnostic "Koa-like" approach that takes any possible Koa context object with whatever arbitrary value it may have for request.method

    • also with the understanding that it is not unreasonable to expect request.method to be manually modified before being passed to koa-router
    • ...and the understanding that custom implementation-specific (and even application-specific) http methods are not uncommon and not unreasonable (vendor-specific CalDAV implementations, ownCloud's WebDAV implementation, etc)
@jamesaspence
Copy link

I agree with this but for a slightly less important reason - I've noticed that autocomplete doesn't work well with these implicit methods. I'd say personally that we should hardcode them as they are rarely going to change, if at all.

@JacobMGEvans
Copy link
Contributor

@jaamison is this PR still in the works, I am interested in seeing it. 😀
Not sure what Koa's core teams feelings are on this @niftylettuce

  • Maybe we can bump this RFC (request for comment) and see if we can get some responses.
    🤔

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

No branches or pull requests

3 participants