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

feat: allow errors from setCookie to be ignored #53

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

krushiraj
Copy link
Contributor

@krushiraj krushiraj commented Jun 2, 2020

Refer this: request/request#794

This is an error from tough cookie and they allow us to ignore errors. Sometimes we don't wanna stop the request when there is an error in cookie setting.

So, now we can allow user to ignore or raise error in setCookie. This will be helpful as most of the people use this package to fetch body from a request and we don't want to end up with an error when there is something wrong in setCookie.

This closes #41 and #52

Refer this: request/request#794

This is an error from tough cookie and they allow us to ignore errors. Sometimes we don't wanna stop the request when there is an error in cookie setting.

So, now we can allow user to ignore or raise error in setCookie. This will be helpful as most of use this package to fetch body from a request and we don't want to end up with an error when there is something wrong in setCookie.
@krushiraj
Copy link
Contributor Author

@valeriangalliat can you please look into this?

@valeriangalliat
Copy link
Owner

valeriangalliat commented Jun 2, 2020

Hey!

The way I would go with that would be more something like this:

const jar = new tough.CookieJar()
jar.originalSetCookie = jar.setCookie
jar.setCookie = (cookieOrString, currentUrl) => jar.originalSetCookie(cookieOrString, currentUrl, { ignoreError: true })

Essentially monkey-patching to makeup for the fact we can't disable errors at the instance level of the cookie jar that we inject into fetch-cookie

That said I don't have strong feelings about that, so if we were to go with an extra argument, I would just make sure to make it an option object so we have more flexibility when we need to forward more options to tough-cookie or anything else, e.g. fetchCookie(fetch, jar, { setCookieOptions: { ignoreError: true } }), what do you think?

Also, any opinion on that @fabiante?

@fabiante
Copy link
Collaborator

fabiante commented Jun 2, 2020

Hi, first of all thanks @krushiraj!

I'd be fine with this implementation. Wrapping the setCookie function may be somewhat cleaner. I thought some time about not having the ignoreError option at all and simply defaulting to wrap the setCookie function with ignoreError set to true if the decorator was created without a custom jar argument. In that case enabling errors would require users of this library to apply the monkey-patch manually. I guess that would become a bit of a hassle and therefore I think I prefer the @krushiraj's change

Concerning a potential setCookieOptions option: Personally, I wouldn't use many of the options that setCookie supports. But directly supporting it won't probably hurt.

@krushiraj
Copy link
Contributor Author

Can you please let me know if there are any final changes that I can do for you? Or is it all good to be merged?

@valeriangalliat valeriangalliat merged commit aad4924 into valeriangalliat:master Jun 16, 2020
@valeriangalliat
Copy link
Owner

Sorry for the delay @krushiraj, I published your PR with version 0.10.0, thanks for the feature!

@krushiraj
Copy link
Contributor Author

Thanks for merging 😄

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

Successfully merging this pull request may close these issues.

None yet

3 participants