ng-cookies

[shahata]

This is an initial suggestion for dealing with the current issues with angular cookies. The main things added by this:

1. An options parameter was added to $cookieStore.put() which supports passing path, domain, expires and secure parameters.
2. $cookieStore was refactored so it will use $browser.cookies() directly instead of going through $cookies. This was in order to allow direct update/read of cookies instead of going through polling/digest mechanism.
3. $cookies was deprecated in order to stop using the polling/digest mechanism and instead getRaw, getAll and putRaw methods were added to $cookieStore in order to accommodate some of the stuff that were previously available only when using $cookies

This is obviously just my take on this and completely open for discussion.

Some open issues:

1. The cookie setting logic is currently located in $browser although it is not used internally by angular (angular only uses the cookie reading logic for XSRF-TOKEN). It might be better to just move it into ngCookies. On the other hand maybe it should be left in $browser for balance, I don't know.
2. Theoretically it is possible to leave $cookies alive and make its properties getters/setters using Object.defineProperty (assuming everyone else agrees that polling/digest mechanism is bad), but we would still need some sort of polling for learning about new cookies. I think we'd better just deprecate it.
3. I'm somewhat uncomfortable with the getRaw/get and putRaw/put duplication. Another option can be to use something like isJsonLike in $http or add a flag to get/put or use try/catch or maybe even configure this behavior in a $cookieStoreProvider, but I don't like it very much. Since getRaw/putRaw are consistent with getAll (which must be raw imo), I like this option best.
4. Might be good to add a $cookieStoreProvider in order to give better defaults for options, I dunno.

[googlebot]

CLAs look good, thanks!

[ghoti143]

I'm curious why the polling/digest mechanism was utilized in the original code. As the $cookies service is non-trivial (i.e. more than syntactic sugar), It seems to be solving some problem (though I am not quite sure what). Just a shot in the dark here...perhaps reading and writing from cookies is a fairly expensive operation in some browsers and delaying the operation until after the current $eval statement will yield a smoother UI?

[shahata]

@IgorMinar did you get a chance to look at the rest of the PR?

[lgalfaso]

I wonder if it would be best to have a super-light version that has the minimum to make $http work at $browser, and move everything else to ng-cookies. In fact, it does not even have to be at $browser, it can be at $http itself

[shahata]

Yeah, I mentioned that in the first "open issue" in my comment above. The cookie writing logic was originally added in $browser only for completeness (imo) and can very easily be moved to ngCookies. It would be a little strange though that the cookie reading is in angular.js and the writing is in angular-cookies.js, I'm not sure. Whether the cookie reading logic is in $browser or $http makes little difference to me, but I think it is better to leave it where it is.

Or are you saying that you want to move the cookie reading logic to ngCookies as well and leave some quick and dirty single cookie reader in $http?

[IgorMinar]

@shahata finished the review of code as is now. some more changes to be done:

• move cookie parsing logic from $browser to ngCookies module
• remove the generic polling functionality from $browser (it's private api so it can be simply deleted)
• the refactoring as it stands today, removes the ability to be notified of cookie changes. This is why the polling functionality was added many years ago. We should consider if it's ok to drop this feature (and document why) or alternatively add the feature back in a more lightweight format (as an opt-in or something)

[shahata]

@IgorMinar per your comments:

1. There is the issue of $http requiring the cookie parsing logic, do you have any suggestion regarding this?
2. Gladly :D Let's just agree on the third item first.
3. Do we have a good use case where someone would want to poll for cookie changes? The way I see it, people were previously able to set a scope.$watch on cookies and count on the poller to trigger digest when they change, but actually cookies only change when the application sets them manually or due to http request (on both option a digest will be triggered anyway, so no need for poller to trigger it). Of course there are cases where people will touch the cookies "out of digest", but I think that it is very rare and those people can write their own polling/notification mechanism...

[shahata]

Okay, I just talked with @IgorMinar about this, here are the conclusions:

1. We are going to remove the cookie polling part completely along with the generic polling mechanism in $browser. We don't see a lot of value in the current polling mechanism - the reason this was originally added was to allow communication between different tabs, but there are better ways to do this today (for example localStorage). If anyone will still need it I guess it is pretty easy to implement the polling on his own. This change and the reasoning behind it needs to be documented.
2. We are going to break the cookie logic into two services: $$cookieReader and $$cookieParser $$cookieWriter . $$cookieReader will remain in angular core (since $http needs it) and $$cookieWriter will move into ng-cookies.
3. We are going to move the cookie API to $cookies instead of $cookieStore and then deprecate $cookieStore (it will still have all of its current API which will invoke $cookies internally just so that we don't break apps that currently use $cookieStore). The new API on $cookies will include the following methods: get, put, getObject, putObject, getAll. This is obviously a breaking change since $cookies will no longer have the cookies as properties like it has today.
4. We'll add an option to set defaults for cookie setting options (path, domain, expires, secure) using $cookiesProvider. This will help people who, for example, want to set all of their cookies as secure.
5. The default for the path option when setting a cookie is the value you have in your <base> tag. This is important since if you load an app at https://myapp.com/my/subview/234, you usually want the cookies to be scoped to the whole app and not to /my/subview/234. This should be documented.

[petebacondarwin]

These conclusions sound about right.

• @lgalfaso suggested that the ngCookies module should be moved into its own file, similar to how ngAnimate works.

[shahata]

This PR is now updated with the conclusions listed above

/cc @IgorMinar @petebacondarwin @lgalfaso

[gkalpak]

These 2 variables seem unused. Could be removed.

[gkalpak]

👍 👍 @shahata

[shahata]

@gkalpak thanks for the comments!
@IgorMinar WDYT?

[pkozlowski-opensource]

@shahata @petebacondarwin @IgorMinar we need to get this in before ng-conf, ideally before the next release. Are there any blocking points left? If so, what are those blocking points and who could help unblocking those?

@shahata the one obvious thing is that the thing doesn't merge cleanly any more. Could you please rebase it? Do we want to squash commits before merging?

[shahata]

@pkozlowski-opensource I've rebased this, thanks. Not sure regarding squashing, it is quite a big change and each commit is an incremental step towards it, so I think it is much easier to understand this way, but if anyone has a strong opinion about this, I don't mind squashing.

@petebacondarwin WDYT? I think it is time to land this... (after travis is green obviously)

[pkozlowski-opensource]

ng-conf is 5 days, just saying :-)

[petebacondarwin]

I'll look at this in the next two days.

[petebacondarwin]

I wonder if we should write a deprecation message to the console when this service is instantiated?

[petebacondarwin]

Nice work @shahata ! Just a bit of tweaking then we can merge this, eh?

TO DO:

• optimize/remove calls to extend
• locally mock $$cookieReader/Writer for $cookie tests but continue to use the actual document.cookie object when testing lower level private $$cookieReader and $$cookieWriter services
• locally mock $$cookieReader for $http tests
• fix up docs

COMMITS
I suggest that we squash this down into the following set of commits, which would make sense to me if viewed in the change log. Did I miss anything?

refact($cookies): split cookie access into $$cookieReader and $$cookieWriter services

fix($cookies): move logic into $cookies and deprecate $cookieStore

The new API on $cookies includes:

• get
• put
• getObject
• putObject
• getAll,
• remove

The put, putObject and remove methods now take an options parameter
where you can provide additional options for the cookie value, such as expires,
path, domain and secure.

DEPRECATION NOTICE:

$cookieStore is now deprecated as all the useful logic
has been moved to $cookies, to which $cookieStore now simply
delegates calls.

BREAKING CHANGE:

The get method now returns the raw cookie value as a string rather than a
deserialized object. Use getObject if this is what you require.

The put method now takes a string, which will be set as the raw cookie value,
rather than an object to be serialized. Use putObject if this is what you require.

Closes #8324
Closes #3988
Closes #1786
Closes #950

fix($cookies): stop polling the browser's cookies values

No longer poll the browser for changes to the cookies and no longer copy
cookie values onto the $cookies object.

The polling is expensive and caused issues with the $cookies properties not
synchronizing correctly with the actual browser cookie values.

The reason the polling was originally added was to allow communication between
different tabs, but there are better ways to do this today (for example localStorage).

BREAKING CHANGE:

$cookies no longer exposes properties that represent the current browser cookie
values. Now you must explictly call get or getObject to access the cookie
values. This also means that you can no longer watch the $cookies properties for
changes to the browser's cookies.

This feature is generally only needed if a 3rd party library was programmatically
changing the cookies at runtime. If you rely on this then you must either write code that
can react to the 3rd party library making the changes to cookies or implement your own polling
mechanism.

Closes #6411
Closes #7631

feat($cookiesProvider): provide path, domain, expires and secure options

This change provides properties on $cookiesProvider so that you can set the application
level default options for cookies that are set using the $cookies service

fix($cookies): set the default path for $cookies from the <base> tag

This is important since if you load an app at
https://myapp.com/my/subview/234, you usually want the cookies to be
scoped to the whole app and not to /my/subview/234.

refact($http): use $$cookieReader for XSRF support

fix($browser): remove private polling mechanism

The only feature of Angular using this mechanism was $cookies,
which no longer mirrors the browser cookie values and so does not
need to poll.

[shahata]

@petebacondarwin @IgorMinar - I fixed and pushed all comments. I'll try to finish squashing tomorrow night.

[petebacondarwin]

Great! Thanks @shahata - if you don't have time to do the squashing I can do this today.

[IgorMinar]

I think you want to add @deprecated annotation to this service

[petebacondarwin]

Closing in favour of #11222