Open kingo55 opened 3 years ago
@dapperdrop and I just caught up to review the cookie enhancements properly. Would you guys have any thoughts to share @awoehrl and @allmywant ?
Here's a brief rundown of what we're thinking:
We set two cookies from the library for user salt and recording assignment:
Cookie name | Purpose |
---|---|
_mjo | User salt (optional) |
mojito{{id}}{{state}} | Assignment record |
We have some undocumented settings for the name prefix, domain and expiry - but we're missing some important new cookies for cross-site/secure usage:
Settings | Supported Features |
---|---|
Cookie name | Yes undocumented, via this.options.options.cookiePrefix |
Domain | Yes undocumented, via this.options.options.cookieDomain |
Expiry | Yes undocumented, via this.options.options.cookieDuration |
Path | No |
Secure | No |
Same site | No |
The original undocumented API was setup a long time ago when there were fewer options we had to support:
Existing undocumented API | Purpose |
---|---|
Mojito.options.cookiePrefix | Sets the name of assignment cookies |
Mojito.options.cookieDomain | Sets the domain of all cookies |
Mojito.options.cookieDuration | Sets the duration of assignment cookies |
We could simply accept an object with settings specified.
Mojito.options.cookie = {
domain: '.foo.bar',
path: '/',
secure: true,
sameSite: 'Lax'
};
Maybe we need to have settings specified for the user salt cookie (because that may have different expiry/naming conventions).
@kingo55 @dapperdrop The proposed API looking good but I think we can expose all cookie options through the API, we can add follow options:
Mojito.options.cookie = {
prefix: 'foo',
expiry: 90, // days
domain: '.foo.bar',
path: '/',
secure: true,
sameSite: 'Lax'
};
Same API can be used for the user salt cookie.
Need to consider backwards compatibility with the existing undocumented API We will need a default cook option object and merge it with the
Mojito.options.cookie
(higher priority) and undocumented APIs and use the merged options for all cookie actions.Do we need to upgrade libraries? Or review new options to migrate to?: https://github.com/js-cookie/js-cookie
js-cookie
looks great, we can consider to migrate to it but it will be a big change to the library if we use js-cookie directly without modifying its APIs.
How can we implement tests to ensure the library performs the right cookie actions?
I think we can write a set of tests write cookies with the proposed option(s) then read and check it.
we can add follow options:
Yes, that's a good idea... I like that. Is it better to represent the cookie expiry in days or seconds?
js-cookie looks great, we can consider to migrate to it but it will be a big change to the library if we use js-cookie directly without modifying its APIs.
Agreed. We haven't had any issues with the current library, so this isn't a big priority.
How can we implement tests to ensure the library performs the right cookie actions?
I think we can write a set of tests write cookies with the proposed option(s) then read and check it.
I wonder whether we can test the cookieDomain/secure in a way that works inside the environments we run tests within (GitHub Actions / Mocha). Most of the other cookie settings should be pretty easy to test for though (path/same site etc).
Is it better to represent the cookie expiry in days or seconds?
I think days is better.
I wonder whether we can test the cookieDomain/secure in a way that works inside the environments we run tests within (GitHub Actions / Mocha).
Ahh this is the problem, it seems we have to tests online, e.g. mintmetrics.io
Cool - yeah, days is much more practical.
I thought the testing environment would be an issue here. Maybe we'd only have to build out the unit tests that we can and manually test anything we can't. Looks like js-cookie
doesn't test those cases either.
For cookies like the Mojito User ID and test assignment, we should allow users to specify the domain and/or behaviour of the cookies (e.g. properties).
It might even be worth updating the cookie library we use too.