expressjs / cookie-parser

Parse HTTP request cookies
MIT License
1.96k stars 220 forks source link

Upgrade cookie to 0.5.0 #92

Closed PezCoder closed 1 month ago

PezCoder commented 1 year ago

Hey! Noticed that the package cookie that we depend on has the latest version available that we can upgrade on which has a few perf benefits.

This although primarily comes from us using cookie-parser in our project which inturns is using an outdated version of the cookie package, while we're on the latest version & not able to de-dupe this to resolve to a single version leading to duplicate versions coming as part of the bundle.

Here's the changelog: https://github.com/jshttp/cookie/releases

Screenshot 2023-03-13 at 4 13 04 PM

Open to suggestions.

MygengBin commented 1 year ago

SameSite set to none is valid

throw new TypeError('option sameSite is invalid');
              ^

TypeError: option sameSite is invalid
    at Object.serialize (D:\work_content\ltd\blog-backend\node_modules\express\node_modules\cookie\index.js:174:15)
    at ServerResponse.res.cookie (D:\work_content\ltd\blog-backend\node_modules\express\lib\response.js:853:36)
    at D:\work_content\ltd\blog-backend\routes\users.js:86:7
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
PezCoder commented 1 year ago

Hey @MygengBin From the changelog I don't think there is any change to SameSite attribute within this version so the issue may be unrelated.

  1. How can I reproduce what you're trying?
  2. Can you try on the latest version of cookie-parser published to see if the issue persists?

SameSite=None support was added in 0.4.0

Make sure your format is correct when sending this:

SameSite=None

Here is the reference to code where cookie package handles it: https://github.com/jshttp/cookie/blob/master/index.js#L195-L213

PezCoder commented 1 year ago

@dougwilson Would you be able to please review this, whenever you can spare some time?

dougwilson commented 1 year ago

Hello 👋! Yes, the cookie module can be bumped, though I don't think it will help woth the samesite problem. I can do it, or if you would like me to merge this PR, just need the commut message to match the other bumps ans also need to add to history.md the changes too.

MygengBin commented 1 year ago

Hey @MygengBin From the changelog I don't think there is any change to SameSite attribute within this version so the issue may be unrelated.

  1. How can I reproduce what you're trying?
  2. Can you try on the latest version of cookie-parser published to see if the issue persists?

SameSite=None support was added in 0.4.0

Make sure your format is correct when sending this:

SameSite=None

Here is the reference to code where cookie package handles it: https://github.com/jshttp/cookie/blob/master/index.js#L195-L213

i am look from cookie-parser, found not that question, but i seen this error path in express , i usually express-generator init project. i found express version is ~4.16.1 my package.json, Oh misunderstanding, i need upgrade my express version. cookie is not ~0.4.0 in that version image

PezCoder commented 1 year ago

Hello 👋! Yes, the cookie module can be bumped, though I don't think it will help woth the samesite problem.

Thank you for the response @dougwilson 🙌🏼 - the intention behind raising this PR was as mentioned in the description & not really related to SameSite attribute, like I highlighted earlier the behaviour for SameSite hasn't changed in this version bump.

I can do it, or if you would like me to merge this PR, just need the commit message to match the other bumps and also need to add to history.md the changes too.

Referring to an older commit: https://github.com/expressjs/cookie-parser/commit/695435aa1af55a35ecc5e7d1fffafb7bc9f7f442

I've made changes to change the commit message, as well as introduced a history.md entry.

Note: I've made a minor version bump as the underlying dependency also has a minor bump, this is because the change in cookies package directly impacts cookie-parser.

Let me know if this looks good, open to further suggestions.

PezCoder commented 1 year ago

A gentle reminder on the review for the suggested changes, whenever you can find the time :) - @dougwilson

PezCoder commented 1 year ago

@dougwilson Hey! Just checking back on this, in the hopes of getting this merged

BogdanCln commented 5 months ago

Hello! This would help us as well, can we please get this merged and released?

UlisesGascon commented 1 month ago

surpassed https://github.com/expressjs/cookie-parser/pull/116