search

jshttp / accepts

Higher-level content negotiation
MIT License
252 stars 42 forks source link

Document `req` Requirements #14

Closed blakeembrey closed 2 years ago

blakeembrey commented 7 years ago

Since this module and negotiator only ever request headers, do you think it'd be reasonable to document and test on this fact? For instance, I'd like to use this module with https://github.com/blakeembrey/node-servie and other non-node HTTP interfaces.

dougwilson commented 7 years ago

I've thought about that a bit, and it is a common request for various module to promise to only use some methods in certain ways. The down side is that once that is specified, it makes the usage of any new property / method on that object a major version bump to the module, defeating the purpose of taking the object in the first place, rather than a more basic type.

I guess my opinion is as follows:

  1. I think accepting req, res, or other objects comes with the expectation that what those are as instanceof http.IncomingMessage or similar and that the module is free to use any methods / properties on that interface as it likes.
  2. An additional, low-level API should be added that asks the caller to provide the data which is a more universal interface than just calling out the specific methods / properties used on an interface someone would just have to completely emulate for no reason.
blakeembrey commented 7 years ago

It's an understandable concern, which is why I'd love to get requirements better documented for others to use. I've had success with HTTP objects where I can just pass in the headers object or the method, and in other cases like busboy it even accepts the stream separately to options (so you can just pass req or a custom stream).

The low-level API though, I'm happy with it looking similar to node HTTP with the exception of passing the stream separately. Most of these utils have clearly defined roles which don't tend to change, which is why it'd be great to document exactly what is used and I don't feel like there'd be much need for major version churn regardless. If there is, it's probably a major change to how the module works already. I definitely agree it should be more general like headers over specific like contentType, so I'm happy to stick with that as that's how I'm using it today anyway.

blakeembrey commented 7 years ago

For the most part, these utilities are actually really usable without req or res. There's only two places it would be easier:

  1. Not assuming (at least in the "low-level") that the properties are set directly on the stream
  2. Documenting the properties in use and either accepting a stream argument or returning a stream to be piped manually
grantila commented 6 years ago

I agree with this. Many of these heavily used packages depending on req/res will be cumbersome to use where you don't really have req/res. I'm currently building a web framework where this pair isn't available. Also, if one wants to use the new http2 module without the compatibility layer, there is no req and res either.

If all you need is the .headers property, and if this is documented, the TypeScript typings will be cleaner too. One won't have to fake an entire http.IncomingMessage or type cast into it, just to use e.g. accepts or negotiator.

XVincentX commented 4 years ago

Just adding that I've ran into the same thing — we're using the module just passing a fake object with the headers inside — even though the object required is a request theoretically.

This is a problem with the types. I can live with it, just adding my two cents :)

blakeembrey commented 4 years ago

Following up on my original issue, I found Hapi's accept package recently which provides a simple interface: https://github.com/hapijs/accept.

dougwilson commented 2 years ago

I know this is an old issue, and I am trying to go through and get everything outstanding addressed. So above there is a suggestion for a module that provides a simplified interface, that is great! As for this module, if there is a desire to add a more lower-level API with more basic types, it would first need to be added to the dependency this module uses -- the issue at hand here is documenting that req could maybe be an object with limited properties, but that object is actually being passed to a dependency of this module, so thinking about it, it would be the wrong layer to make such a limitation at.