Skip to content

Fix: add ability to create Partitioned Cookies#226

Merged
mcollina merged 7 commits intofastify:masterfrom
nicob-29:Partitioned-Cookie-Bis
Dec 8, 2023
Merged

Fix: add ability to create Partitioned Cookies#226
mcollina merged 7 commits intofastify:masterfrom
nicob-29:Partitioned-Cookie-Bis

Conversation

@nicob-29
Copy link
Copy Markdown
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
Copy Markdown
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?

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
nicob-29 and others added 3 commits December 4, 2023 20:11
Co-authored-by: Gürgün Dayıoğlu <gurgun.dayioglu@icloud.com>
Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
@nicob-29
Copy link
Copy Markdown
Contributor Author

nicob-29 commented Dec 5, 2023

@Eomm , Tests added

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
@nicob-29
Copy link
Copy Markdown
Contributor Author

nicob-29 commented Dec 5, 2023

@Uzlopak Thanks !

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

Uzlopak commented Dec 6, 2023

@Fdawgs

PTAL

@gurgunday
Copy link
Copy Markdown
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
Copy Markdown
Member

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

Co-authored-by: Gürgün Dayıoğlu <gurgun.dayioglu@icloud.com>
@jsumners
Copy link
Copy Markdown
Member

jsumners commented Dec 6, 2023

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

@gurgunday
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Uzlopak commented Dec 6, 2023

Is the title now better?

Copy link
Copy Markdown
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
@nicob-29
Copy link
Copy Markdown
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