Add useCookie mixin
#478
Add useCookie mixin
#478
Conversation
There was a problem hiding this comment.
Thank you so much for working on this @divagueame.
This looks good overall, but I think there are some nuances that I personally would implement differently to be closer to the other Stimulus/Stimulus-use patterns. Take a look and let me know what you think. Thank you!
docs/use-cookie.md
Outdated
| // You can save new cookies with setCookie | ||
| // By passing two strings as name-value | ||
| this.setCookie("dessert", "cake") |
There was a problem hiding this comment.
I'm wondering if it would be more natural from an API point of view, and also for symmetry, to use a more Stimulus-like way to set cookies. Something like:
| // You can save new cookies with setCookie | |
| // By passing two strings as name-value | |
| this.setCookie("dessert", "cake") | |
| // You can save new cookies with the [name]Cookie setter | |
| this.desertCookie = "cake" |
Or was there a reason against this?
docs/use-cookie.md
Outdated
| // Getters are initialized for all current cookies. | ||
| // For a cookie with name 'colorScheme' and value 'dark' | ||
| this.colorSchemeCookie // dark |
There was a problem hiding this comment.
I'm not sure if this is the best behavior. I can technically see how this is useful, but this somehow goes against the other Stimulus/Stimulus use patterns.
I think it would make sense if you have to explicit tell the mixin the cookie names you want to use. This makes it less magical and easier to follow the flow, especially if other people on the team want to understand the controller. But also for us, so we can appropriately generate the right getters and setters for all the defined cookies.
|
hey, thanks for the feedback. Both issues you commented on are kind of related. First, I implemented the API like you suggested I thought it would be useful to skip that step an do it magically : ) However, I can understand the benefits of keeping it consistent and explicit, so I can rewrite like that. |
|
Hi, I refactored the PR to follow a more similar API to the rest of the library. Let me know what you think. |
This PR adds use-cookie to stimulus-use. I tried to keep it as simple as possible and to provide the most basic functionality in regards of could be needed to easily work with cookies on the client.
I added some tests with the core functionality and added an example on the playground. I tried to test if cookies were expiring as expected but I could not get the Sinon time fakers working properly, so eventually I gave up and decided to put it out there. Maybe someone has a better idea of how get properly tested.
Glad to hear any feedback.