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

Upgrade cookie to 0.5.0 #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PezCoder
Copy link

@PezCoder PezCoder commented Mar 13, 2023

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

  • I ran the tests locally & they seem to have been passing.

Open to suggestions.

@MygengBin
Copy link

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
Copy link
Author

PezCoder commented Apr 7, 2023

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
Copy link
Author

PezCoder commented Apr 7, 2023

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

@dougwilson
Copy link
Contributor

dougwilson commented Apr 7, 2023

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
Copy link

MygengBin commented Apr 8, 2023

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 PezCoder force-pushed the upgrade-cookie-minor-version branch from d26d176 to 1571e78 Compare April 10, 2023 17:15
@PezCoder
Copy link
Author

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: 695435a

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.

HISTORY.md Outdated Show resolved Hide resolved
@PezCoder
Copy link
Author

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

@PezCoder PezCoder force-pushed the upgrade-cookie-minor-version branch 3 times, most recently from af71e39 to b788193 Compare April 23, 2023 06:10
@PezCoder
Copy link
Author

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

@expressjs expressjs deleted a comment from KingPerson Jun 11, 2023
HISTORY.md Outdated Show resolved Hide resolved
@PezCoder PezCoder force-pushed the upgrade-cookie-minor-version branch from b788193 to 5a0e0ca Compare June 17, 2023 08:10
Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

LGTM! This should be merged and released before #105

@UlisesGascon UlisesGascon changed the title Upgrade cookie minor version to 0.4.2 -> 0.5.0 Upgrade cookie to 0.5.0 May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants