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

Implement loose mode for cookie jars #56

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Implement loose mode for cookie jars #56

merged 1 commit into from
Oct 6, 2015

Conversation

Sebmaster
Copy link
Contributor

Forgot that this might be needed too, especially if tough-cookie is used in conjunction with request, which doesn't set loose by default.

I was thinking about doing an options object as a param instead, but I don't think that the already existing option can be replaced in a backwards-compatible way, so we could only add an options object in addition. Not sure about what to do here.

@Sebmaster
Copy link
Contributor Author

This might not be needed if request/request#1811 gets merged, but just in case we want to provide this functionality anyways...

@stash-sfdc
Copy link
Contributor

Nice, yes, this is needed.

If we're going to change the signature of CookieJar's constructor, I think it should be (store, options) instead of (store, rejectPublicSuffixes, looseMode). It should have been that way originally, in hindsight. Either way it's a SemVer MINOR bump. Can be backwards compatible, e.g.:

function CookieJar(store, options) {
  if (typeof options === 'boolean') {
    options = { rejectPublicSuffixes: options };
  }
// ...
  this.rejectPublicSuffixes = options.rejectPublicSuffixes

Could you also document the change in README.md, please and thank you?

@Sebmaster
Copy link
Contributor Author

Implemented. It's not quiiite backwards compatible since before you didn't have to pass a boolean arg and it'd still work.

Locally it seems like master failed before this, which also now causes the PR to fail. This seems to not be a case on Travis though?!
There's something weird about this test format though, so I'm not quite sure why that is. Seems like there's an Error object in the second argument instead of the first one here.

@stash-sfdc
Copy link
Contributor

Did you update vows locally? I bumped to using 0.8.1 on master.

Aside: TODO: stop using vows 😉. It was a good choice at the time tough-cookie was created, but I find the simplicity/stability of mocha (or even TAP) to be qualitatively, and opinionatedly, better.

@Sebmaster
Copy link
Contributor Author

Yep, that was it. Successful here too then 👍

stash-sfdc added a commit that referenced this pull request Oct 6, 2015
Implement loose mode for cookie jars
@stash-sfdc stash-sfdc merged commit 2b93e5e into salesforce:master Oct 6, 2015
@stash-sfdc
Copy link
Contributor

Oops, hold on, didn't push something so will re-merge.

@stash-sfdc
Copy link
Contributor

ok, should be good. will publish as 2.2.0 shortly

@stash-sfdc
Copy link
Contributor

Published! Thanks again @Sebmaster, you rock!

@Sebmaster Sebmaster deleted the feature/loose-cookiejar branch October 7, 2015 01:58
@Sebmaster
Copy link
Contributor Author

Nice, hopefully I can finally put this 2 year old bug I found to bed 😀

wjhsf pushed a commit that referenced this pull request Feb 8, 2024
* Prep for prerelease, LWC to 0.37.4

* platformResourceLoader mocks return promise

* expectedApiVersion to 46.0

* jest@24.8.0

* 0.5.0

* update CHANGELOG

* Upgrade LWC to 0.37.4-220.2, remove deasync (#56)

* Remove deasync section of README
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.

2 participants