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: add ability to create Partitioned Cookies #226

Merged
merged 7 commits into from
Dec 8, 2023

Conversation

nicob-29
Copy link
Contributor

@nicob-29 nicob-29 commented Dec 4, 2023

Related to #213

  • run npm run test and npm run benchmark
    npm run benchmark -> failed
    TypeError: redisStoreFactory is not a function
    at createServer (C:\GitHub\fastify-session\benchmark\bench.js:19:24)
    at testFunction (C:\GitHub\fastify-session\benchmark\bench.js:56:18)
    at main (C:\GitHub\fastify-session\benchmark\bench.js:83:15)

  • tests and/or benchmarks are included

  • documentation is changed or added

  • commit message and code follows the Developer's Certification of Origin
    and the Code of conduct

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

COuld you add a test?

package.json Outdated Show resolved Hide resolved
lib/cookie.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Manuel Spigolon <[email protected]>
lib/cookie.js Outdated Show resolved Hide resolved
nicob-29 and others added 3 commits December 4, 2023 20:11
Co-authored-by: Gürgün Dayıoğlu <[email protected]>
Co-authored-by: Aras Abbasi <[email protected]>
@nicob-29
Copy link
Contributor Author

nicob-29 commented Dec 5, 2023

@Eomm , Tests added

test/cookie.test.js Outdated Show resolved Hide resolved
Co-authored-by: Aras Abbasi <[email protected]>
@nicob-29
Copy link
Contributor Author

nicob-29 commented Dec 5, 2023

@Uzlopak Thanks !

Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

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

The PR title needs to be rewritten as it's not great in its current state. Won't make much sense to users reading release notes.

@nicob-29 nicob-29 changed the title Partitioned Bis (Fix Merge Conflict) Partitioned Cookie Dec 5, 2023
@nicob-29 nicob-29 requested a review from Fdawgs December 5, 2023 13:57
@gurgunday
Copy link
Member

What is wrong with this CI?

Rerunning

@Uzlopak Uzlopak changed the title Partitioned Cookie Fix: Use Partitioned Cookie by default Dec 6, 2023
@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 6, 2023

@Fdawgs

PTAL

@gurgunday
Copy link
Member

gurgunday commented Dec 6, 2023

It's not enabled by default?

Also, I wonder why we're not using fastify/cookie's cookie module here

@gurgunday
Copy link
Member

And we might want to label it as experimental or non-standard

README.md Outdated Show resolved Hide resolved
Co-authored-by: Gürgün Dayıoğlu <[email protected]>
@jsumners
Copy link
Member

jsumners commented Dec 6, 2023

I am completely against making a draft proposal the default implementation of anything.

@gurgunday
Copy link
Member

Yeah me too, the title is wrong, the PR just adds partitioned, it doesn't make it the default

@Uzlopak Uzlopak changed the title Fix: Use Partitioned Cookie by default Fix: add ability to create Partitioned Cookies Dec 6, 2023
@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 6, 2023

Is the title now better?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 77e9847 into fastify:master Dec 8, 2023
19 checks passed
@nicob-29
Copy link
Contributor Author

nicob-29 commented Dec 8, 2023

Thank you everyone !

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.

7 participants