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

Limit the tough-cookie to 2.5 as 3.x removes node 0.10 support #299

Merged
merged 3 commits into from
Feb 14, 2019

Conversation

aomdoa
Copy link

@aomdoa aomdoa commented Jan 13, 2019

Latest tough-cookie does not have node 0.10 support. Update the package.json to limit to the 2.5 version.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1457338 on aomdoa:fixToughCookieDep into 18c838a on request:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1457338 on aomdoa:fixToughCookieDep into 18c838a on request:master.

@jrjohnson
Copy link

jrjohnson commented Jan 20, 2019

Just a note that the latest tough-cookie version also drops node 6 which is not yet EOL. This bump to 3.0.0 should probably have been a breaking change in request-promise, but the >= constraint let it slip through.

@analog-nico
Copy link
Member

Hey @aomdoa @jrjohnson 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?

@jrjohnson
Copy link

I'm using neither tough-cookie or resolve-promise directly. My chain looks like

└─┬ ember-percy@1.5.0
  └─┬ percy-client@3.0.3
    ├─┬ request@2.88.0
    │ └── tough-cookie@2.4.3 
    └─┬ request-promise@4.2.2
      └── tough-cookie@3.0.1

The issue is that a breaking change of tough-cookie bubbled up through the chain to be a breaking change in my library because the version constraint >=2.3.3 allowed it through. In my experience it is pretty rare for a library like request-promise to have a >= version constraint, but it does put you in a position where you have to stay up to date with your dependencies and then manage new releases which is annoying.

@analog-nico analog-nico added the Bump Patch Bump the patch version once released label Feb 14, 2019
@analog-nico analog-nico merged commit c535eb6 into request:master Feb 14, 2019
@analog-nico
Copy link
Member

Thank you very much @jrjohnson ! That makes sense. Well, ideally percy-client would take care of this but since this is surely not the only package where we run into it, it would be a bottomless pit to open issues for these projects.

To ensure that even possible edge cases are gone once you install the next release that I’ll do shortly, please check afterwards that request and request-promise don’t have separately installed versions of tough-cookie anymore and instead tough-cookie is physically only installed once.

@analog-nico
Copy link
Member

request-promise@4.2.3 is released which includes this fix.

@jrjohnson
Copy link

Thanks @analog-nico! I agree that I could have avoided this (and did in the end) by just not updating my lock file so this transitive dependency update didn't hit my app. I'll check what is being installed when I next update. Would you like me to submit a PR that updates tough-cookies to the 3.0 series so it can be released here as a breaking 5.0?

@analog-nico
Copy link
Member

Thanks again for your input @jrjohnson ! No need for a PR. Thanks. The important point here is that request and request-promise must use the exact same physical copy of the lib because it otherwise fails to do an importance instanceof comparison. Currently, request uses @~2.5.0 so tough-cookie@3 is not relevant yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bump Patch Bump the patch version once released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants