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.

Sign up for GitHub

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

feat(#78): allow cookieExpiresAfterDays as a function #79

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat(#78): allow cookieExpiresAfterDays as a function #79

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f960398
Allow cookieExpiresAfterDays to be a function
gtnsimon Jan 12, 2022
908cafe
fix: let user take care to return a value when using a function
gtnsimon Jan 13, 2022
e027077
chore: documentation to use it as a function
gtnsimon Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/consent-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,24 @@ export default class ConsentManager {
saveConsents() {
if (this.consents === null) deleteCookie(this.cookieName);
const value = this.config.stringifyCookie(this.consents);
const configCookieExpiresAfterDays = this.config.cookieExpiresAfterDays;

let cookieExpiresAfterDays =
typeof configCookieExpiresAfterDays === 'function'
? configCookieExpiresAfterDays(this.consents, this.config)
: configCookieExpiresAfterDays;

if (
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe remove this test and use a default value in setCookie like before.
This way any falsy or 0 value would be set to the default.

setCookie(
	this.cookieName,
	value,
	cookieExpiresAfterDays || 120,
	this.config.cookieDomain
);

Copy link
Contributor

Choose a reason for hiding this comment

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

cookieExpiresAfterDays could then be a const var.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with falsies values but 0 as an integer has a meaning for cookie expiration which ends when browser is closed. Using cookieExpiresAfterDays || 120 will replace 0 to 120 which may not be the expected behavior by the end developer.

To not break existing installations, the default could apply to non-functional cookieExpiresAfterDays (like before this feature). When used as a function it'll be up to the developer to explicitly return a value if he don't want a session cookie.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about this behavior with 0, given the old implementation I thought it was kind of invalid.
I'm kind of torn here, it would be a breaking change to modify the current behavior, but it seems odd to have different behaviors for different methods...

cookieExpiresAfterDays === null ||
cookieExpiresAfterDays === undefined
) {
cookieExpiresAfterDays = 120;
}

setCookie(
this.cookieName,
value,
this.config.cookieExpiresAfterDays || 120,
cookieExpiresAfterDays,
this.config.cookieDomain
);
felixgirault marked this conversation as resolved.
Show resolved Hide resolved

Expand Down