Skip to content

Conversation

@Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 14, 2023

This way we can use it in fastify/secure-session and co instead of the cookie package.

Checklist

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

Great, thanks

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 15, 2023

@mcollina PTAL

@mcollina
Copy link
Member

Plase require the entrypoint file in

const cookie = require('../cookie')
.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

If the other module is going to use @fastify/cookie to manage cookies, why do we need to export the underlying tooling?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 20, 2023

@mcollina
PTAL

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.

Can you document it in the README?

@gurgunday gurgunday requested a review from mcollina November 24, 2023 14:49
README.md Outdated
you should register it prior to any other `onRequest` hooks that will depend
upon this plugin's actions.

It is also possible to import the low-level cookie parsing and serialization functions.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a link here to the header section regarding the import?

@gurgunday gurgunday requested a review from Fdawgs November 25, 2023 13:57
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 25, 2023

@mcollina PTAL

@gurgunday
Thank you for your work.

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

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.

6 participants