-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
refactor the CookieJar interface for more implementation independence #56
base: main
Are you sure you want to change the base?
refactor the CookieJar interface for more implementation independence #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leptopoda Thanks for your contribution, and of course they are welcomed!
Regarding the breaking part, a detailed migration guide would be helpful for the process. Also you may want to check the compatibility with dio_cookie_manager
and other downstream packages to learn their cases.
About RFCs, it would be great if you could ref them as comments directly in the code, so we can always follow them.
93416bc
to
b952da4
Compare
Sorry it has been a while since the last review. Could you fix all the tests? |
The linting is failing, not the tests |
Yeah and the lints too |
Signed-off-by: Nikolas Rimikis <[email protected]>
…plementation Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
…ies as defined by RFC6265 Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
b952da4
to
08b66c4
Compare
Sorry for my inactivity. The CI should be passing now. |
Hi there and thank you for this package.
I hereby suggest a few changes to the interface of the CookieJar class. We wanted to implement a custom cookie storage backend in sql and ended up writing a custom CookieJar instead of just using a custom
Storage
.We did observe that the api is quite biased towards the
DefaultCookieJar
implementation and not really considering other implementations.We decided to go with:
FutureOr
ignoreExpires
that not every cookie store needs (we strive for RFC compliance so we do not need the option to ignore the expiration).delete
with adeleteWhere
function so users of the api can decide with higher precision what cookies they want to be removed.endSession
function to remove non persistent cookies as described in the RFCThis is just a proposal and we are open to talk about changes. We hope you consider these changes even if they are breaking. Obviously there are less breaking ways to achieve this like keeping around the
delete
function and just deprecating it for now.We are also open to upstream the rest of our RFC compliance focused code if you are open and interested in further changes.