ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Make it possible to set cookies client-side #25670

Open morsssss opened 4 years ago

morsssss commented 4 years ago

We hear this request often from more sophisticated and experienced AMP site creators - to allow reading and setting of cookies from <amp-bind>, <amp-list>, and/or <amp-analytics>.

A common workaround is to set cookies server-side, which sometimes makes it necessary to create an API to set cookies. We want AMP to make things easier for developers, and unfortunately this server-side workaround makes the experience harder for developers and users alike.

Of course, being able to set cookies from <amp-script> would be nice too, and might cover these use cases. But it would be great to add this to older interactive components as, of course, they're far more widely adopted at the moment.

One possible format:

on="tap:AMP.setCookie({currency: 'USD'})"

/cc @mattwomple , @SimonHogstromRonbol, @nainar, @choumx

alankent commented 4 years ago

Cookie support would be great! Example use cases I have bumped into include:

Need to be clear on what restrictions exist for AMP Cache vs origin domain names in URLs and cookies. A cookie set on the origin will be not available if a page is served from the AMP Cache?

Side note: Elsewhere I had wondered if it made sense to associate AMP state with cookies. AMP state is initialized to selected cookies on page load (making it accessible from to render after initial page load). Setting AMP state bound to cookies updated the cookie.

morsssss commented 4 years ago

/cc also @mdmower

@nainar , do you think we can prioritize this? Might it even make a Good First Issue™️?

mdmower commented 4 years ago

Implementing AMP.getCookie() and AMP.setCookie() is easy enough, but a design review may be necessary to determine which directives are configurable. I don't mind starting an I2I if @ampproject/wg-runtime doesn't outright forbid implementation of this feature request. Let me know.

samouri commented 4 years ago

I don't see a problem with this, but I'm not a good authority on Cookies. @jridgewell, thoughts on allowing AMP.setCookie?

jridgewell commented 4 years ago

Needs a privacy review.

morsssss commented 4 years ago

Makes sense. How do we kick that off?

jridgewell commented 4 years ago

Search for SPUR in moma, we have to file ariane tickets, etc.

alankent commented 4 years ago

I don't know enough about how AMP.getCookie() would be made available, but just a reminder I think a valuable use case is for the cookie to affect rendering of the page on initial page load. E.g. personalization - inject the user's name if returning, or display preferences based on segmentation knowledge from previous sessions etc.

This is why I liked the idea of saying "persist this part of AMP state in a cookie". If the AMP state is updated, update the cookie. On page load, restore that AMP state from the cookie before #26473 kicks in feeding AMP state into amp-list rendering on page load. (I am sure there are other ways of doing it, but getting cookie values on page load into amp-list or similar I think is valuable. I want the original page render affected by the cookie value.)

morsssss commented 4 years ago

+1 to that idea too - the ability to serialize a state var into a cookie, then reverse the process on subsequent page load

mdmower commented 4 years ago

@morsssss @alankent Serialized states are generally too large to store in cookies (rule of thumb is 4K for all cookies on a domain). I think your suggestion would be better accommodated by giving publishers access to localStorage.

morsssss commented 4 years ago

I hadn't been thinking about such large state variables! localStorage is an interesting idea...

mdmower commented 4 years ago

I put together a draft PR for what is probably the simplest approach to adding AMP.setCookie support. I'm finding that cookie support is pretty low on my priority list of interests though, so I'd like to encourage someone else to take on the privacy and design reviews. Feel free to hijack as much or as little of https://github.com/ampproject/amphtml/pull/27397 as you like.

As for getCookie support, my suggestion would be to create a separate I2I. Start simple, like adding cookie('abc') as a supported function in amp-bind (admittedly not trivial... all bind functions evaluate in a worker without access to document). This would allow cookie values to be assigned to states.

dreamofabear commented 4 years ago

Thanks Matt.

As a meta point, I wonder if promotion of cookies might be a future foot-gun WRT on-cache vs. off-cache, given the direction of 3P cookies e.g. https://webkit.org/blog/10218/full-third-party-cookie-blocking-and-more/. Though there's still value for AMP first/dirty AMP/signed exchanges.

morsssss commented 4 years ago

This is a good point. Cookies were tough before in the world of the AMP cache, and I don't think we're going to see signed exchanges everywhere anytime soon.

On the other hand, we are trying hard to promote the idea of AMP-first! And I think the number more of people who will struggle with AMP because they can't use cookies is greater than the number who will have trouble with cookies vanishing on the cache.

So I'm personally in favor of pursuing this. If others aren't, though - if we think cookies are going to become a dinosaur - I think we should simply be public about that in our documentation and outreach.

morsssss commented 4 years ago

Noticing this again... is there interest in following this through to completion? It's one of those features that developers frequently request, and it unlocks the door to a number of website features.

Looks like @mdmower gave this a great head start here: https://github.com/ampproject/amphtml/pull/27397

mdmower commented 4 years ago

@morsssss Out of curiosity, are developers more often requesting "get cookie" or "set cookie" functionality? If "set cookie" is sufficient, it's mostly just a matter of running #27397 through a privacy review (admittedly, not a small task, but requires less design time than a "get cookie" feature).

morsssss commented 4 years ago

Sadly my contact with developers isn't what it was before March 🙄 As I think about requests for this that I heard last year and the prior year, I don't feel like I heard people asking for just get or set. I remember requests related to analytics, requests related to persisting state without having to make server calls... and simple client-side personalization. I also remember that @alankent was interested in remembering logged-in state.

If those are still the use cases, "set cookie" will make some of that functionality easier, so it's probably better than nothing :)

Of course it might be easier to do this with amp-script: https://github.com/ampproject/worker-dom/issues/451

codysaylor commented 4 years ago

You can do a lot with cookies/localstorage, but I'd like to use either to simply remember preferences a user selects such as a preferred dark/light mode css, so I'd love to see this feature implemented.

morsssss commented 4 years ago

Makes sense!

@codysaylor , if it helps for the time being, amp-script does support local storage.

bilalmalkoc commented 2 years ago

Hello. Any news about cookies?

morsssss commented 2 years ago

I think we'd need a volunteer to follow up on @mdmower 's exploration.

mdmower commented 2 years ago

This is a bit speculative, but I imagine the reason cookies-for-AMP hasn't gotten traction is creators would be disappointed by limitations. Reading and writing cookies would likely need to be restricted to source origins (e.g. allowed on example.com, but not on google.com/amp/s/example.com). If all AMP pages served from AMP caches were delivered as signed exchanges (wouldn't that be cool?!), we'd be all set. As it is though, with so many AMP pages viewed from google.com/amp, this cookie feature would lead to differences in capabilities between pages served via an AMP viewer and those served from source origins.

EDIT: I suppose I should have re-read this thread before posting. This concern has been mentioned a few times 😏 . So, nothing new here... but probably valid nonetheless.

morsssss commented 2 years ago

it's still an important point @mdmower ! 😎

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.