Skip to content

feat: use tough-cookie@5.1.x directly #2453

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sampsonjoliver
Copy link

@sampsonjoliver sampsonjoliver commented Mar 13, 2025

Mirror of #2396, updating tough-cookie from ^5.0 to ^5.1.

As per that discussion, it appears the only necessary change is to update tough-cookie to 5.1.x in order to effectively drop the @bundled-es-modules/tough-cookie clone.

pnpm build succeeds against 5.1.x where it fails against 5.0.x.

@kettanaito
Copy link
Member

Thanks for opening this one, @sampsonjoliver. Looks good to me, let's just make sure that this update also doesn't break any CJS consumers.

@kettanaito
Copy link
Member

Looks like ESM consumers are broken:

  1) esm-browser.test.ts:61:5 › runs in an ESM browser project ─────────────────────────────────────

    Error: expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 4

    - Array []
    + Array [
    +   "The requested module './../../../../../../tough-cookie@5.1.2/node_modules/tough-cookie/dist/cookie/index.js' does not provide an export named 'Cookie'
    + ",
    + ]

Reproduction:

pnpm test:modules:browser

@sampsonjoliver
Copy link
Author

sampsonjoliver commented Mar 13, 2025

Hm, I recall in #2356, it was said:

In september with the release of v5 tough-cookie was rewritten in typescript and (I'm assuming here) is probably compatible with ES without this wrapper package.

I wonder if this assumption has been validated! Perhaps we still need the esm wrapper 🤔

I may have time to look into this during my AM hours tomorrow, or else later next week.

@sampsonjoliver
Copy link
Author

sampsonjoliver commented Mar 17, 2025

Hey @kettanaito, I started looking into the module:browser tests but couldn't really make heads or tails of it, in part due to unfamiliarity with the test suite and the way it's bundling/generating the module code.

I can see when running the test suite with --ui that there is a network request to get the tough-cookie module, shortly before the error is thrown to the console:

URL: http://localhost:62644/node_modules/.pnpm/tough-cookie@5.1.2/node_modules/tough-cookie/dist/cookie/index.js
Method: GET
Status Code: 200

and the response body matches the file in node_modules, including an export definition for Cookie like so:

var cookie_1 = require("./cookie");
Object.defineProperty(exports, "Cookie", { enumerable: true, get: function () { return cookie_1.Cookie; } });

What is interesting is that I seem to be able to modify cookieStore.ts to completely remove the import of tough-cookie (replacing instances of its members with {} as any), then run pnpm build, run the tests again, and get the same error.

There should be no reference to tough-cookie in the code under test, yet the same error is produced. This is the question that's currently stumping me - where is the attempted import coming from?

@sampsonjoliver
Copy link
Author

Also, this may be of some relevance, though it does not solve my question above: salesforce/tough-cookie#506

@kettanaito
Copy link
Member

kettanaito commented Mar 24, 2025

The error seems to indicate that the cookie package is CJS-only. To properly import it, we need to do a wrapper import like this:

import cookiePkg from 'tough-cookie'

cookiePkg.Cookie
cookiePkg.Store
// etc.

I believe we have one or two packages we import the same way in the code base. If I may ask you to look around and try this one locally, that would be highly appreciated!

Thank you for putting so much work into this.

Edit: Checked the code quickly and no, we've dropped such imports (likely migrated to ESM wrappers for that reason). There are references online though.

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.

3 participants