mainmatter / ember-cookies

Cookies abstraction for Ember.js that works both in the browser as well as with Fastboot on the server
MIT License
115 stars 46 forks source link

Feature Request: Add expires shortcuts? #54

Closed justinaray closed 7 years ago

justinaray commented 7 years ago

carhartl/jquery-cookie and js-cookie both provided a shortcut to expiration. If you provided expires: <Number>, then it would assume an expiration of days. Would you be agreeable to this sort of feature in ember-cookies?

We could also enhance this significantly by supporting a String value specifying a duration and units. Maybe use the same characters as date formatting to specify units? ex: '3d' or 50M for 3 days or 50 minutes respectively

Thanks for the addon! Loving it and killing off a jquery plugin and bower dependency!

marcoow commented 7 years ago

I'm not sure this is something that's actually needed to be honest, especially as we'd just be adding shortcuts that's already very simple, e.g. For 3 days:

3 * 24 * 60 * 60

Also if you treat expires: 3 as 3 days how would you distinguish that from a 3 second expiry?

justinaray commented 7 years ago

Completely agree that it's trivial, but you do have to pass a Date instance for expires, right? So it's a bit more involved.

this.get('cookies').write(MY_COOKIE_NAME, cookieVal, {
...
expires: new Date(Date.now() + 3 * 24 * 60 * 60 * 1000) // Raw Date approach
expires: moment().add(3, 'days').toDate() // Moment approach
...
}

Again, this is pretty straightforward to write, but I saw it and immediately thought of a utility so I suggested it. To me, this seems easier:

this.get('cookies').write(MY_COOKIE_NAME, cookieVal, {
...
expires: '3d' // 3 days
...
}

Completely up to you as a maintainer, but just thought I'd suggest it. Thanks again for the addon!


Re: Number param

I also agree that a Number value for expires is ambiguous. I just suggested days as a default as it's consistent with the other cookie libraries. I guess they assume standard cookie duration is in days? Had I designed the api for those other libraries, I would have probably supported a String variant with value/units as I suggested above.

marcoow commented 7 years ago

but you do have to pass a Date instance for expires, right?

Good point ;) I guess allowing to pass a number (and treating this as "seconds from now") would actually be a good idea then.

I'd actually not want to allow strings like '3d' - if we allow passing seconds I think there's very little need for something like this and we'd add a custom vocabulary that mostly adds unnecessary complexity to the library. The main idea behind ember-cookies is to provide a cooke abstraction that works both in the browser as well as on the server and not really to provide a fancy API on top of cookies.

justinaray commented 7 years ago

Seconds seems like a good and flexible default to me! Easy to do the maths from there as you initially commented.

Would you prefer to overload expires as we've been chatting about or add another option key for the seconds config?

I am pretty swamped with work as I am sure you are as well. Just wanted to hash out the api in case I find some time this weekend to look into a PR.


Aside (Again working through the API)

Millis are usually the standard time manipulation unit, but since cookies only have second precision, I think that's perfectly fine to use seconds as the default here and not to go to that level of precision in this API.

marcoow commented 7 years ago

Would you prefer to overload expires as we've been chatting about or add another option key for the seconds config?

I think overloading should be fine.

Millis are usually the standard time manipulation unit, but since cookies only have second precision, I think that's perfectly fine to use seconds as the default here and not to go to that level of precision in this API.

max-age is in seconds as well (see https://developer.mozilla.org/en-US/docs/Web/API/document/cookie) so I think seconds should be fine.

geekygrappler commented 7 years ago

Hey, @justinaray we have maxAge option, which takes a time in seconds, so I've documented that on the Readme. Does that work as an alternative to overloading expires?

justinaray commented 7 years ago

Thanks @geekygrappler and @marcoow. Max-Age will work for our use case, seems to have pretty good browser support, and MDN says it will be preferred over Expires if both are set and supported.

👍