valeriangalliat / fetch-cookie

Decorator for a `fetch` function to support automatic cookie storage and population. 🍪
The Unlicense
135 stars 29 forks source link

Handle request object correctly #40

Closed RiftLurker closed 4 years ago

RiftLurker commented 5 years ago

Properly fixes #28.

Before this change this module wasn't able to properly retrieve the cookie string which resulted in it setting an empty one instead.

The naming looks quite confusing now (url.url), it might be a good idea to rename the parameters (you might take inspiration from TypeScript), though that's not my call to make.

RiftLurker commented 5 years ago

Applying this fix to my local version of fetch-cookie inside my project solved the issue for me and so does calling the wrapped fetch directly (without a Request object). Though I'm currently unsure whether this is an appropriate fix and if this is actually an issue in this module as I'm unable to create a test that demonstrates the broken behavior. Both fetch-cookie and tough-cookie are able to handle my request object properly but it's clearly not working inside my project.

fabiante commented 5 years ago

LGTM, but I having a test to show the fixed misbehaviour would be nice IMO.

While you are at it, I think it would be a good idea to rename the url parameter to request and add a JSDoc like:

  /**
   * @param url {string|Request}
   * @param opts
   * @returns {Promise<*>}
   */
RiftLurker commented 5 years ago

I absolutely agree on the test, at my first attempts it looked like there was another reason as to why it wasn't working in my project and couldn't be isolated into a failing test. Since not using a Request object works for me at the moment, I won't be able to spend much more time on this, might have to revisit in the future.

If you want to merge without a test, let me know so I can rename the parameters, otherwise feel free to close the PR.