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

fix: target tough-cookie 2.x to preserve node 6 support #33

Merged
merged 3 commits into from
Feb 14, 2019
Merged

fix: target tough-cookie 2.x to preserve node 6 support #33

merged 3 commits into from
Feb 14, 2019

Conversation

jasonmit
Copy link
Contributor

@jasonmit jasonmit commented Jan 9, 2019

Tough-cookie now ends up resolving to 3.0.0 (published today) which drops support for Node 6.

Node 0.12.0 and iojs builds are failing but not because of this PR.

@@ -146,7 +146,7 @@ describe('Request-Promise-Native', function () {
var cookiejar = rp.jar();

expect(function () {
cookiejar.setCookie(sessionCookie, 'https://api.mydomain.com');
cookiejar.setCookie(sessionCookie.toString(), 'https://api.mydomain.com');
Copy link
Contributor Author

@jasonmit jasonmit Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: request/request-promise#277

Gets the CI back to being green for modern node versions. Likely a similar issue plagues Node 0.12.0 and iojs builds.

package.json Outdated
"stealthy-require": "^1.1.0",
"tough-cookie": ">=2.3.3"
"stealthy-require": "^1.1.1",
"tough-cookie": "^2.5.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 049152b on jasonmit:u/jasonmit/fix-node-6 into 1874877 on request:master.

@coveralls
Copy link

coveralls commented Jan 9, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 9cfcaaa on jasonmit:u/jasonmit/fix-node-6 into 1874877 on request:master.

package.json Outdated
},
"peerDependencies": {
"request": "^2.34"
"request": "^2.88.0"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonmit if you update the peer dep here you will need to update the dev dep below too. but why did you update the peer dep in the first place? is it required for the fix to work? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, removed! (Left over fragment as I was debugging the broken CI pipeline)

@Turbo87
Copy link

Turbo87 commented Jan 14, 2019

@analog-nico do you have time to look at this in the next days? seems like this is breaking CI for quite a lot of packages lately, especially incombination with salesforce/tough-cookie#140, which essentially now retroactively makes this project support only Node 8 and above 😞

@jasonmit
Copy link
Contributor Author

/cc @mikeal since you're still active and part of the org. Any chance you have publishing rights?

@analog-nico
Copy link
Member

Hey @jasonmit @Turbo87 quick question: My assumption is that tough-cookie is only used by your own code. Thus you will install it yourself in your project and with that define the version of the library that you use. request-promise is lenient here because it shall use the same version of tough-cookieas you use in your own code. Is my assumption right or did I miss something?

@Turbo87
Copy link

Turbo87 commented Feb 14, 2019

the problem is that npm and yarn will install the latest compatible version, ignoring the Node.js compatibility. means if you run Node 6 it would try to install the latest version, which needs Node 8+ and then the install will fail.

@analog-nico analog-nico merged commit c2c54c4 into request:master Feb 14, 2019
@analog-nico
Copy link
Member

Thank y’all!

request-promise-native@1.0.6 is released which includes this fix.

@jasonmit jasonmit deleted the u/jasonmit/fix-node-6 branch February 15, 2019 02:58
@Turbo87
Copy link

Turbo87 commented Feb 15, 2019

awesome, thanks! do you have access to the other request-promise packages too? I think they had similar problems

@analog-nico
Copy link
Member

My pleasure @Turbo87 ! Yes, I updated all packages in this family at the same time.

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.

4 participants