search

jhermsmeier / node-http-link-header

Parse & format HTTP link headers according to RFC 8288
MIT License
39 stars 12 forks source link

Make parse fn robust when undefined #24

Open binarykitchen opened 3 years ago

binarykitchen commented 3 years ago

When you do something like LinkHeader.parse(undefined) it crashes with:

Uncaught TypeError: Cannot read property 'replace' of undefined

I'd make this a bit more robust and would just return undefined as well.

binarykitchen commented 3 years ago

And maybe add some test cases for any funny parameters like null, true, false, non-strings etc.

jhermsmeier commented 3 years ago

You can also just check what you're passing in. Not sure what the value would be of being able to pass in something that won't parse anyway without it throwing. What's the use case?

binarykitchen commented 3 years ago

Passing further the response from any API to your library in the middle of a chain. If the response has no links, is undefined, then I expect the parser to return nothing as well instead of crashing.

jhermsmeier commented 3 years ago

What'd be your expectation of behavior for a malformed link header, for example? Currently that throws. Now, that'd mandate a try/catch around link header parsing anyways, which is why I'm not sure I understand why null / undefined is an issue. Would you expect malformed values to throw (as is currently the case), or also return null / undefined? What's the use case for it behaving differently for any other unparseable value? Differentiating between no value and malformed value?

jaydenseric commented 3 years ago

A simple runtime type validation for the constructor or methods that accept a Link header string would make it much clearer what is going on if the input you're attempting to pass in is wrong, e.g:

if (typeof value !== 'string') throw new TypeError('Argument 1 `value` must be a string.');

I ran into a situation where Next.js was mocking Node.js response header related methods, and the header values were always an empty object:

https://github.com/vercel/next.js/blob/e27b7e996d6e7e6772272f3d189ce93dac25fc3d/packages/next/export/worker.ts#L169

Which is a strange behavior, since they should only ever be a string, or array of strings. I had to debug http-link-header internals to figure out what was wrong; if the constructor and parse method threw a type error I would have known right away what was wrong.

melroyvandenberg commented 1 month ago

We got a similar issue here with: cannot read properties of undefined (reading 'replace'). It would be nice if this package can handle these issues better instead of throwing this random error message.