[JS] Replace jQuery.cookie with JS-cookie and provide a BC layer

I'm working at this issue at DrupalCon Nola in the sprint room!

This looks like a good idea!

ckeditor might have a reference here: /core/assets/vendor/ckeditor ... it's minified code so I'm going to see if it's implementation is--in fact--a jquery one.

I'm tagging this 'need subsystem review' because I'm not sure if we can just pull out this javascript library (i.e. modules might depend on it)

There is a CKEditor reference to a cookies object in the codebase, but on examination of the un-minified CKEditor code, it is a native (i.e. non-jquery) implementation. The other reference was in the joyride library, but on examination of the code (as droplet said), it appears to be a soft dependency-- as it should be, since the JQuery cookies plugin has been deprecated. In my testing of the tour module, the jquery cookies function was not called, and the tour module appears to work perfectly fine.

jquery.cookie library is no longer maintained.
https://github.com/carhartl/jquery-cookie

Mh, are you sure this can still be removed? Technically this would be a BC break and well, contrib modules / custom modules might easily rely on its existence.

@droplet
Is that a better title?

A ticket for 7: #2742789: Update Cookie plugin 1.0

Once again, agreed with @droplet in #10

Here is a patch swapping it for js-cookie.

Still to do adding BC layer - though I'll leave that to someone with more in depth JS knowledge... unless you're happy to walk me through it of course.

In my opinion however I don't see why core would have to provide this library if itself is not using it. We could provide this in contrib, and other modules depending on jquery.cookie could move to that... perhaps in 9.x?

Oops, thanks tests!

@droplet re #10:

Both jQuery.cookie & JS-cookie shared same API design. we can add BC layer

It's probably worth noting that while sharing the same syntax, jquery.cookie and js-cookie have different defaults: $.cookie('name', 'value') sets a cookie for the current path but Cookies.set('name', 'value') sets the cookie for the root.

We can use js-cookie v1.5.1 as reference for the BC layer as it was back compatible with jquery-cookie at that point.

https://github.com/js-cookie/js-cookie/blob/v1.5.1/src/js.cookie.js

The release notes v2.0.0 also contains some great notes on what we need to do on our BC layer.

https://github.com/js-cookie/js-cookie/releases/tag/v2.0.0

There was 2 years ago however a subsystem maintainer signed of on this, probably should revisit if this library is the correct replacement :)

Based on #17 and #26 this is at Needs work to add a backwards-compatibility layer.

I updated the issue summary and changed the priority to Major.

The patch no longer applied so I re-rolled and then tried to implement a backwards-compatible "core/jquery.cookie" library. I also added the "needs frontend framework manager review" based on @johndevman's comment regarding it being 2 years since the last subsystem maintainer review.

1. Updates to the latest stable release of js-cookie 2.2.1 and switched to the minified version. One caveat is that the 3.0.0 release is imminent and that hasn't been evaluated.
2. Changes "core/jquery.cookie" library into one that adds $.cookie and $.removeCookie wrappers.
3. The "raw" and "json" properties are set on the $.cookie object.
4. The setter does not implement a converter because that is not the behavior of jquery.cookie. The getter always implements a converter that either uses a passed in jquery.cookie converter callback either with the raw or decoded value. This is because passing in a converter will always use the raw value in js-cookie, which is different from the original jquery.cookie behavior.
5. I ran npm run lint:core-js and npm run build.

I didn't add a backwards-compatible way to set path based on the 2.0.0 release notes.

To manually test, I applied the "with-joyride" version of the patch, enabled the Tour module, which still has a reference to jquery.cookie, and then went to the View Edit page. In my console (Firefox), I was able to run the various commands to test basic functionality. The "with-joyride" version should not be used because joyride does not have a hard dependency on jquery.cookie.

jQuery.cookie("blah"); returns undefined.
jQuery.cookie("blah", "blah"); returns "blah=blah; path=/".
jQuery.cookie("blah"); returns "blah".
jQuery.cookie("blah", "halb"); returns "blah=halb; path=/".
jQuery.removeCookie("blah"); returns true.
Cookies.get("blah"); returns undefined.
Cookies.set("blah", "blah"); returns "blah=blah; path=/".
Cookies.get("blah"); returns "blah".
Cookies.set("blah", "halb"); returns "blah=halb; path=/".
Cookies.remove("blah"); returns undefined.

I added a question in the proposed resolution to clarify the the needs frontend framework manager review tag.

Thank you @mradcliffe! This seems like great progress!

Since we don't have any test coverage for this ourselves, I was thinking if we could run the tests from the upstream package. I think I figured steps needed to be able to do that, but it seems like there's some failures that we might want to look into.

Steps I used for running tests:

• git clone git@github.com:carhartl/jquery-cookie.git
• git apply --patch jquery-cookie-test-only.patch
• yarn install
• ./node_modules/.bin/grunt connect:tests

I'm tagging this for subsystem maintainer review because JavaScript maintainers might want to review this too. 😎✌️

1. switched issue to 9.0.x and rerolled

2. +++ b/core/core.libraries.yml
   @@ -229,7 +229,6 @@ drupal.form:
   -    - core/jquery.cookie
   
   @@ -289,7 +288,6 @@ drupal.tabledrag:
   -    - core/jquery.cookie
   
   @@ -425,7 +419,6 @@ jquery.joyride:
   -    - core/jquery.cookie
   

   I am not sure how we can remove the dependencies here and no tests fail. I am probably missing something but since these libraries depend on core/jquery.cookie and we are removing it without a replacement for a dependency shouldn't functionality be lost? Is it just because we don't have test coverage?

3. +++ b/core/core.libraries.yml
   --- /dev/null
   +++ b/core/misc/jquery.cookie.es6.js
   

   Could we actually name this with "shim" or something in the file name. We should provide a BC layer for someone using the library not the file directly. This seems confusing.

4. +++ b/core/misc/jquery.cookie.es6.js
   @@ -0,0 +1,53 @@
   + * @deprecated in Drupal 8.9.0 and will be removed in Drupal 10.0.0. Use the
   + * core/js-cookie library directly instead.
   

   I think we should also be calling Drupal.deprecationError() for all of these methods. see #3070521: Trigger deprecation notifications on JavaScript deprecations to notify developers that deprecated code is being used

5. The shim is pretty complicated I think we should tests that it works

Forgot to upload patch

I am not sure how we can remove the dependencies here and no tests fail. I am probably missing something but since these libraries depend on core/jquery.cookie and we are removing it without a replacement for a dependency shouldn't functionality be lost? Is it just because we don't have test coverage?

Based on the research earlier in the issue, it was determined that these were not hard dependencies. This _could_ affect contrib/custom if someone depended on having the cookie plugin added to JQuery, but only depended on drupal/form or drupal/tabledrag. There isn't any use of the dependencies in those two.

Could we actually name this with "shim" or something in the file name. We should provide a BC layer for someone using the library not the file directly. This seems confusing.

This makes sense to me.

I think we should also be calling Drupal.deprecationError() for all of these methods

+1. Would this go in each function in the shim or called once when the immediately-invoked function expression (IIFE) is invoked?

Updated the issue summary.

Forgot to change to Needs Work (NW).

@mradcliffe

1. +1. Would this go in each function in the shim or called once when the immediately-invoked function expression (IIFE) is invoked?

   I think in each function in the shim. This would be the first non-test to use this but you can take a look at core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js

2. Based on the research earlier in the issue, it was determined that these were not hard dependencies.

   thanks for the explanation. I wonder if there is any way we could deprecate this dependency. It shouldn't be in this issue but it seems like we should only have dependencies for what core needs and if someone need to set a cookie they should add the library explicitly. Not sure how we could deprecate something like and be able to warn sites.

I renamed the shim to include shim, added the Drupal.deprecationError calls instead of @deprecated, and then added a cute test module and Nightwach test for both the shim and js-cookie. But I probably have screwed up my Drupal JavaScript behaviors since I haven't done that in a while.

Uploading the patch, but it still needs work.

1. +++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_shim_test.es6.js
   @@ -0,0 +1,16 @@
   +      $.find('.js_cookie_test_add_button').on('click', () => {
   ...
   +      $.find('.js_cookie_test_remove_button').on('click', () => {
   
   +++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_test.es6.js
   @@ -0,0 +1,16 @@
   +      $.find('.js_cookie_test_add_button').on('click', () => {
   ...
   +      $.find('.js_cookie_test_remove_button').on('click', () => {
   
   +++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_test.js
   @@ -0,0 +1,21 @@
   +      $.find('.js_cookie_test_add_button').on('click', function () {
   ...
   +      $.find('.js_cookie_test_remove_button').on('click', function () {
   

   I tried to change this to

   $('.js_cookie_test_add_button').once('add').on('click', but that still didn't work in Nightwatch.

   However that did work when I made a custom module that did the same thing and installed locally for manual testing.

2. +++ b/core/modules/system/tests/modules/js_cookie_test/js_cookie_test.libraries.yml
   @@ -0,0 +1,16 @@
   +    - core/jquery-cookie
   

   Should be core/jquery.cookie.

   I fixed this in this patch.

3. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,52 @@
   +      .click('.js_cookie_test_add_button')
   +      .getCookie('js_cookie_test', result => {
   

   My guess is that Nightwatch's async. is too fast here. Maybe click takes a function?

4. +++ b/core/modules/system/tests/modules/js_cookie_test/js_cookie_test.libraries.yml
   @@ -0,0 +1,16 @@
   +    js/jquery_cookie_shim_test.js: {}
   

   Also fixed this file name to match the real file name.

+++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_shim_test.es6.js
@@ -0,0 +1,20 @@
+      $('.js_cookie_test_add_button')
+        .once('add')
...
+      $('.js_cookie_test_remove_button')
+        .once('remove')

+++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_test.es6.js
@@ -0,0 +1,20 @@
+      $('.js_cookie_test_add_button')
+        .once('add')
...
+      $('.js_cookie_test_remove_button')
+        .once('remove')

Switching to once requires core/jquery.once dependency although that seems to be inherently required on standard/minimal installs (local dev) rather than testing installs (Nightwatch).

I think my original issue was with $.find rather than $, but using once is the right thing to do here.

This did expose a test failure with my current assertion.

This addresses my comment in #40, and now the js-cookie test is working.

Also I found that I forgot to delete jquery.cookie.js in #38's patch, and that core.libraries.yml entry for jquery.cookie had that file name referenced, which is why the shim test is failing.

This should fix the patch and pass the tests now. :-)

Hmm, I forgot to actually add js-cookie to the patch because I had a .gitignore active on this environment, which restricts "vendor" anywhere.

+++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
@@ -0,0 +1,54 @@
+  'Test js-cookie': browser => {
+    browser
+      .drupalRelativeURL('/js_cookie_test')
+      .waitForElementVisible('.js_cookie_test_add_button', 1000)
+      .click('.js_cookie_test_add_button')
+      .getCookie('js_cookie_test', result => {
+        // js-cookie stores cookie values encoded, and Nightwatch reads the
+        // cookies without decoding.
+        browser.assert.equal(decodeURIComponent(result.value), 'red panda');
+      })
+      .waitForElementVisible('.js_cookie_test_remove_button', 1000)
+      .click('.js_cookie_test_remove_button')
+      .getCookie('js_cookie_test', result => {
+        browser.assert.equal(result, null);
+      })
+      .drupalLogAndEnd({ onlyOnError: false });
+  },

I don't think we need this test just the one that covers the shim. I don't we generally provide test for third-party libraries we are just including.

The shim has it's logic so the test there is good.

This is looking good, all I've found so far are no-big-deals.

1. +++ b/core/core.libraries.yml
   @@ -205,7 +205,6 @@ drupal.form:
   -    - core/jquery.cookie
   

   It may be good to get input from @lauriii, as he has a good sense of how BC-supporting the libraries layer needs to be. Removing these includes could be a potential BC problem in 8.9. Even though these libraries no longer use cookie functionality, there may be custom/contrib uses that are able to access jQuery cookie because these common libraries happen to be loaded.

2. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,53 @@
   +(function($, Drupal, cookies) {
   
   +++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_shim_test.es6.js
   @@ -0,0 +1,20 @@
   +(function({ behaviors }, $) {
   
   +++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_test.es6.js
   @@ -0,0 +1,20 @@
   +(function($, { behaviors }, cookies) {
   

   These should use arrow syntax

3. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,53 @@
   +  const deprecatedMessage = `This function is deprecated in Drupal 8.9.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library directly instead.`;
   

   This can be moved to the scope of $.cookie()

4. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,53 @@
   +  const isFunction = obj =>
   ...
   +  const reader = (s, converter, isRaw) => {
   ...
   +  $.cookie = (key, ...args) => {
   

   These functions should be annotated. In particular it would be good for the descriptions to be detailed enough that if someone thinks there's an issue with the shim, this will help them confirm or refute that suspicion. (It'll also help me do a more thorough review of the JS - everything looks solid so far but I'd like to be sure that assessment isn't based on incorrect assumptions).

5. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,53 @@
   +    if (value !== undefined && !isFunction(value)) {
   

   This is another spot where a comment will help facilitate a quicker understanding of what is going on.

6. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,53 @@
   +})(jQuery, Drupal, window.Cookies);
   

   Somewhere I think it would be useful to make it clear that window.Cookies originates in js-cookie. Largely to benefit contributors that may not spend much time in JS.

7. +++ b/core/modules/system/tests/modules/js_cookie_test/src/Controller/JsCookieTestController.php
   @@ -0,0 +1,54 @@
   +class JsCookieTestController {
   

   Doesn't look like there's anything problem-causing about this approach, but there could be some future-proofing in matching the common convention and extending ControllerBase instead of using StringTranslationTrait directly.

8. +++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_shim_test.es6.js
   @@ -0,0 +1,20 @@
   +(function({ behaviors }, $) {
   

   Needs arrow syntax

9. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,53 @@
   +(function($, Drupal, cookies) {
   ...
   +  const reader = (s, converter, isRaw) => {
   ...
   +
   

   This should use arrow syntax

10. +++ b/core/modules/system/tests/modules/js_cookie_test/js_cookie_test.info.yml
    @@ -0,0 +1,5 @@
    +version: version
    

    All-caps the value for version:

I haven't re-rolled the patch in a while so this might fail to apply. I wanted to work on the changes in the review by @bnjmnm #44 and @tedbow #43, which are mainly all correct. Thank you!

The patch addresses:

• Removes the non-shim test and replaces with a test for JSON support: #43
  • I didn't try to adding a test for either the raw property or a converter/read callback (js-cookie/jquery.cookie respectively).
  • This new test is failing at the moment. I don't have time at the moment to debug, but getJSON is not working wit the shim. I'm glad I added that new test :-)
• Using arrow syntax: 44-2, 44-8, and 44-9.
• Provides jsdoc documentation: 44-4, 44-6
  • I found that this somewhat usable when I tried to generate the docs with jsdoc with jQuery being a namespace. That's probably out of scope of this change, and there aren't really any core examples where we're doing this already.
  • Cookies shows up as a global now, but it won't show up as a global (similar to jQuery) when it is removed in 10.0.0. That is out of scope because generating jsdoc with the vendor components doesn't work at the moment for jQuery.
• Tried to clarify the comment with getter/setter logic from jquery.cookie: 44-5
  • I think that the jsdoc param helps explain this as well
• Extends ControllerBase for the test controller: 44-7
• Fixes version capitalization: 44-10
• Fixes a missing argument when passing a js-cookie style read converter. This isn't really necessary since jquery.cookie's converter only uses one argument.
• Fixes calling a js-cookie style converter with a read property. This isn't really necessary since it's not part of jquery.cookie.

Regarding 44-3:

This can be moved to the scope of $.cookie()

I didn't do this because the deprecation message is used in both $.cookie and $.removeCookie, and I don't think I need to define another const with the same text.

Patch does not address 44-1.

Issue status remains at Needs work.

This patch addresses the bug in the getter that was exposed when I looked at the JSON test. I had a good test for the setter, but not the getter since I was asserting based on the raw cookie value returned from Nightwatch. That is still important to assert that it is encoded.

In addition to changing that assertion, I added direct calls to jQuery.cookie in the Nightwatch to test the output of the getter in both tests. Nightwatch suggests not passing arrow syntax into its execute method so I needed to disable some eslint style rules for those lines.

I also removed passing in a js-cookie style converter because the shim should only support what was supported by jquery.cookie, which was a function.

44-1 is pending frontend framework manager review so I'm adding that issue tag back.

I created a "test only" patch and I think this should not fail on core since the test module is only testing the shim and not js-cookie. Hopefully. :-)

Oh, wait, it's going to fail because the deprecated message won't be there.

However the first test does pass up until that point which asserts the encoded and decoded cookie value :partyparrot:

Removing the deprecated assertions from a test-only patch works up until a change from how js-cookie 2 handles decode/encode from jquery.cookie. The raw cookie value is allowed to contain brackets unencoded in js-cookie, but not in jquery.cookie. This shouldn't matter if using JSON because the object is returned correctly, but if manually encoding/decoding than it is a BC break. It is a very minor one though, and it could have been considered a bug fix because the change was to only encode unallowed characters in the cookie value rather than any URI encoding.

However the safest thing to do is to use the decode/encodeURIComponent from jquery.cookie, and then document this behavior as deprecated in the change record. The JSON test needs to be updated to assert that the cookie value is fully encoded for BC.

I wasn't successful in fixing the bc break yet, but I'm uploading my work-in-progress patch as well as an update test-only patch that should pass on 8.9.x or 9.0.x or whatever as-is.

It took a bit more than expected in my previous comment. The encoding went through the parseCookieValue function in jquery.cookie rather than just decodeUriComponent.

In this patch there is an issue in the converter.write as it is either not using it for JSON or something else.

From Slack, @laurii mentioned "Is there any downsides in keeping those dependencies? Maybe we could move removing the dependencies to a follow-up?"

I don't think there are any downsides to doing #44-1 (other than confusion N years from now), and definitely we could remove them in 10.0.0. I'm going to remove the review tag.

I needed to use withConverter in the setter as well as the getter. :-)

Since that's the case, I reverted some changes in #50 in the withConverter.

This now passes jsCookieTest locally for me.

I create a change record.

This is looking very good!

This isn't a complete review yet as I've mainly only looked through the shim JS. I'm providing what I have so far as I may have spotty internet access for a bit.

1. +++ b/core/modules/system/tests/modules/js_cookie_test/js/js_cookie_shim_test.es6.js
   @@ -0,0 +1,26 @@
   +        .once('add')
   ...
   +        .once('remove')
   ...
   +        .once('add')
   

   I notice that that 'add' is used as an arg for two instances of once(), which doesn't seem to be a problem in this case, but it can't hurt to reduce overall the number of calls to once() by wrapping the click handler additions in a single

         if ($('body').once('js_cookie_test-init').length) {
     // add the three listeners without using once()
   }
   

2. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,200 @@
   +  const writer = cookieValue => encodeURIComponent(cookieValue);
   

   I noticed this is called only once and just calls to encodeURIComponent(), perhaps I'm overlooking a specific reason for using this vs not calling encodeURIComponent() directly?

3. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,200 @@
   +  const decodeCookieValue = value => {
   +    if (value.indexOf('"') === 0) {
   +      value = value
   +        .slice(1, -1)
   +        .replace(/\\"/g, '"')
   +        .replace(/\\\\/g, '\\');
   +    }
   +    return decodeURIComponent(value.replace(/\+/g, ' '));
   +  };
   

   Since this is largely a refactored-for-Drupal-coding-standards copy of parseCookieValue() from jQuery cookie, could this get renamed to parseCookieValue(), accompanied by an explanation of what is different about this function vs the one in jQuery cookie (including why the try block was removed). This makes it easier to confidently review since it'll be apparent it's logic that has been in use in Drupal for many years.

4. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,200 @@
   +   * // Returns 'myCookie=myCookieValue'.
   ...
   +   * // Returns 'myCookie={"key": "value"}'.
   

   The return values for a setter call are not entirely accurate since it appends the attribute values ex: "myCookie=myCookieValue; path=/". This is mentioned in the description for @return but could be clearer in these examples.

5. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -3,27 +3,106 @@
   +   * $.cookie('.myCookie');
   

   Remove the leading dot from '.myCookie'

6. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -3,27 +3,106 @@
   +   * @param {...Array} args
   +   *   Additional arguments to pass into the function when used as a setter.
   +   *   The second function argument is the value of the cookie to be set, or
   +   *   when used as a getter, is a js-cookie converter or callback
   +   *   third function argument is an object of additional options. See the
   +   *   js-cookie library README.md for more details.
   

   Since there are only two potential elements and they're immediately assigned to variables in the function, perhaps it's more readable to use

   (key, value, options). That makes it possible to do a more straightforward jsDoc for each @param.
   </li>
   
   <li>
   <code>
   +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -3,27 +3,106 @@
   +   *   Additional arguments to pass into the function when used as a setter.
   +   *   The second function argument is the value of the cookie to be set, or
   +   *   when used as a getter, is a js-cookie converter or callback
   +   *   third function argument is an object of additional options. See the
   +   *   js-cookie library README.md for more details.
   

   A few of the lines in this comment can have one additional word. I noticed the line breaks happen at the 79th character in a few spots so I'm guessing this is based on the cursor position after the final character, which can be 81 since the character appears in column 80.

7. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -3,27 +3,106 @@
   +   * @return {string}
   +   *   Returns the cookie name, value, and other properties based on the
   +   *   return value of the document.cookie setter.
   

   'return' can go on the previous line

A few additional things to finish up the review:

1. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,200 @@
   +  const writer = cookieValue => encodeURIComponent(cookieValue);
   

   In my previous comment I mentioned that this may not need a dedicated function -- now I'm wondering if the intent was to add a condition based on $.cookie.raw, since jQuery cookie checks the value of raw when encoding?

.

2. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,200 @@
   +   * @prop {boolean} json
   +   *   True if the cookie value should be returned as-is without decoding
   +   *   URI entities.
   

   'URI' can get moved to the prior line.

3. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,200 @@
   +   * Removes a JavaScript cookie.
   

   This is technically removing a browser cookie.

4. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,200 @@
   +   *   Returns true when the cookie is undefined.
   

   Perhaps change "undefined" to "successfully removed"

5. I was curious what might happen to cookies that were set with jQuery cookie, then continue to be present when switching to use the shim. The encoding is different. I'm not sure offhand what kind of problems this could cause or if it can even be avoided, but this should at least be mentioned in the change record.
   This is the console output of what I did to check this.

   // set cookie on branch without shim
   jQuery.cookie('myCookie', { key: 'value' });
   "myCookie=%5Bobject%20Object%5D"
   jQuery.cookie('myCookie');
   "[object Object]"
   // apply patch with shim
   jQuery.cookie('myCookie');
   "[object Object]"
   // Set the same cookie with shimmed version
   jQuery.cookie('myCookie', { key: 'value' });
   "myCookie=%7B%22key%22%3A%22value%22%7D; path=/"
   jQuery.cookie('myCookie');
   "{"key":"value"}"
   

6. As seen in the console output of the previous item, the shim version of $.cookie('key', 'value') returns a differently formatted string than when calling jQuery Cookie. I did a search of http://grep.xnddx.ru/ to see if the return value of cookie setting is used in any contrib and there are no instances of it. At the very least the change record should mention this, but it would still be ideal for the shim to return the same format of string so it's truly backwards compatible.

This patch addresses the minor standard review fixes and attempts to make the shim more backwards-compatible with jquery.cookie quirks.

The raw parameter had two functions in jquery.cookie:

1. When setting a cookie, it would not uri encode the cookie name, but had nothing to do with the cookie value.
2. When getting a cookie, it would not uri decode the cookie name and value.

The reason why the encoding is different in 55-4 and 55-5 is that the json option wasn't used and an object is passed in. In jquery.cookie, values are typecast to string unless the json option is true. js-cookie has a different behavior to always parse JSON, and if the output made sense, use it. To solve this, the shim will now only pass in an object if the json option is true, otherwise, the cookie value will be the string representation of the value, which matches jquery.cookie.

The patch does not address the path of the cookie. The default option should probably be to not use js-cookie's default '/' for the path. This is needs review, but is still pending because of that. The draft change record still needs to be updated to manually set path.

This patch reverts path to always be an empty string to be backwards-compatible. The behavior of path when empty differs between Firefox and Chrome, and I was unable to write a Nightwatch test to assert the path using ChromeHeadless. The path always came back as / even though I manually tested in the browser and checked storage that the path was set to the current page correctly.

#57 has some JS test failures:

[Example Test] Test Suite
15:34:12 =========================
15:34:16 Running:  Test page
15:34:16 
15:34:16 ✔ Element <body> was visible after 31 milliseconds.
15:34:16 ✔ Testing if element <body> contains text: "Test page text"  - 34 ms.
15:34:16 
15:34:16 OK. 2 assertions passed. (299ms)
15:34:16 Running:  Page objects test page
15:34:16 
15:34:16 ✔ Element <body> was visible after 22 milliseconds.
15:34:16 ✔ Testing if element <Element [name=@body]> contains text: "Test page text"  - 33 ms.
15:34:16 ✔ Ensuring no deprecation errors have been triggered  - 19 ms.
15:34:16 
15:34:16 OK. 3 assertions passed. (150ms)
15:34:17 
15:34:17 [Js Cookie Test] Test Suite
15:34:17 ===========================
15:34:22 ✔ Element <input[name="modules[js_cookie_test][enable]"]> was visible after 540 milliseconds.
15:34:24 Running:  Test jquery.cookie Shim
15:34:24 
15:34:24 ✔ Element <.js_cookie_test_add_button> was visible after 28 milliseconds.
15:34:24 ✔ Passed [equal]: $.cookie returns decoded cookie value
15:34:24 ✔ Passed [equal]: Cookie value is encoded backwards-compatible with jquery.cookie.
15:34:24 ✔ Testing if "This function is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library directly instead." deprecation error has been triggered  - 6 ms.
15:34:24 ✔ Element <.js_cookie_test_remove_button> was visible after 27 milliseconds.
15:34:24 ✖ Failed [equal]: (cookie value is not set) - expected "null" but got: "[object Object]"
15:34:24     at NightwatchAPI.Test jquery.cookie Shim.browser.drupalRelativeURL.waitForElementVisible.click.execute.getCookie.assert.deprecationErrorExists.waitForElementVisible.click.getCookie.result (/var/www/html/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js:52:24)
15:34:24     at <anonymous>
15:34:24     at process._tickCallback (internal/process/next_tick.js:189:7) 
15:34:24 
15:34:24 
15:34:24 FAILED: 1 assertions failed and  5 passed (300ms)
15:34:24 
15:34:24 [Js Deprecation Test] Test Suite
15:34:24 ================================
15:34:29 ✔ Element <input[name="modules[js_deprecation_test][enable]"]> was visible after 539 milliseconds.
15:34:31 Running:  Test JavaScript deprecations
15:34:31 
15:34:31 ✔ Element <body> was visible after 26 milliseconds.
15:34:31 ✔ Testing if element <h1> contains text: "JsDeprecationTest"  - 33 ms.
15:34:31 ✔ Testing if "This function is deprecated for testing purposes." deprecation error has been triggered  - 6 ms.
15:34:31 ✔ Testing if "This property is deprecated for testing purposes." deprecation error has been triggered  - 5 ms.
15:34:31 
15:34:31 OK. 4 assertions passed. (145ms)
15:34:31 
15:34:31 [Login Test] Test Suite
15:34:31 =======================
15:34:35 Running:  Test login
15:34:35 
15:34:36 ✔ Expected element <.user-role-form .machine-name-value> to be visible in 2000ms - condition was met in 537ms
15:34:38 ✔ User "user" was created successfully  - 36 ms.
15:34:39 ✔ Passed [equal]: The user "user" was logged in.
15:34:39 ✔ Element <body> was visible after 25 milliseconds.
15:34:39 ✔ Testing if element <h1> contains text: "Reports"  - 33 ms.
15:34:39 ✔ Ensuring no deprecation errors have been triggered  - 9 ms.
15:34:39 
15:34:39 OK. 6 assertions passed. (3.928s)
15:34:39 
15:34:39 [States Test] Test Suite
15:34:39 ========================
15:34:44 ✔ Element <input[name="modules[form_test][enable]"]> was visible after 541 milliseconds.
15:34:47 Running:  Test form with state API
15:34:47 
15:34:47 ✔ Element <body> was visible after 26 milliseconds.
15:34:47 ✔ Element <input[name="textfield"]> was not visible after 24 milliseconds.
15:34:47 ✔ Ensuring no deprecation errors have been triggered  - 23 ms.
15:34:47 
15:34:47 OK. 3 assertions passed. (148ms)
15:34:47 _________________________________________________
15:34:47 
15:34:47 TEST FAILURE:  1 assertions failed, 26 passed. 35.061s
15:34:47 
15:34:47  ✖ jsCookieTest
15:34:47  – Test jquery.cookie Shim (300ms)
15:34:47    Failed [equal]: (cookie value is not set) - expected "null" but got: "[object Object]"
15:34:47        at NightwatchAPI.Test jquery.cookie Shim.browser.drupalRelativeURL.waitForElementVisible.click.execute.getCookie.assert.deprecationErrorExists.waitForElementVisible.click.getCookie.result (/var/www/html/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js:52:24)
15:34:47        at <anonymous>
15:34:47        at process._tickCallback (internal/process/next_tick.js:189:7)
15:34:47    SKIPPED:
15:34:47    - Test jquery.cookie Shim With JSON
15:34:47 
15:34:47 error Command failed with exit code 5.

I'm not sure why this is now failing on drupalci. It still passes locally for me. I went and applied the patch manually again too. :(

I don't think concurrency is an issue here because it's the same test.

I'm wondering if this is an async problem. I tried to convert this to use a BDD type expect which would retry until certain time has passed. Unfortunately, it seems like Nightwatch doesn't have API for checking if cookie exists or not so we would have to add that ourself. I opened upstream issue so that this could be added there too eventually https://github.com/nightwatchjs/nightwatch/issues/2304.

It seems like we're not using the same default values in the $.remove with $.cookie. This seems to work fine for some reason when Drupal is being run on root path, but DrupalCI runs Drupal in a subdirectory. I could reproduce the problem manually while running Drupal in a subdirectory. This should fix the tests too.

Tagging as a requirement for Drupal 9.0-beta1.

Could we expand the test coverage to be closer to what is covered by https://github.com/carhartl/jquery-cookie/blob/master/test/tests.js? I'm still concerned that this change would end up changing behavior in some edge cases. Writing test coverage seems like the only way to catch changes in those.

Updated patch with multiple tests based on jQuery.cookie tests coverage.

small quickfix

locally that test was passed. not sure why.

@finnsky did you try running tests when Drupal is installed in a subpath of the host? That was the reason for the DrupalCI failures in #57.

@lauriii i just runned it on local php server as described here
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/.env.example#L11

Thank you for moving this issue forward @lauriii and @finnsky. I found a couple of minor issues.

1. +++ b/core/.eslintrc.json
   @@ -15,6 +15,7 @@
   +    "Cookies": true,
   

   Patch doesn't apply anymore because of #3086369: Deprecate matchMedia. Needs re-roll. I was able to apply the patch with

   git apply -v -3 <PATCH> and accept both changes.

2. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,256 @@
   +const getJqueryCookie = function(cookieName) {
   

   I ran core:lint-js and got the following

   core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
     2:25  warning  Unexpected unnamed function  func-names

   I think this needs the eslint override and the comment that explains why we need Nightwatch to deviate from the code standards.

3. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,256 @@
   +  if (cookieName) {
   

   I didn't get the same error in test bot, but I did get a test error when I tried running locally.

   TypeError: Error while running "getCookie" command: Cannot read property 'value' of null

   I think this is because that this is not expected to be a boolean or string. It needs an explicit undefined and/or null check in JavaScript.
   This would be fine in TypeScript.

4. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,256 @@
   +    return jQuery.cookie(cookieName);
   +  }
   +  return jQuery.cookie();
   

   Maybe also

   const getJqueryCookie = function(cookieName) {
     return cookieName !== null && cookieName !== undefined
       ? jQuery.cookie(cookieName)
       : jQuery.cookie();
   }
   

5. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,256 @@
   +      .getCookie('js_cookie_test_remove', result => {
   +        browser.assert.equal(result.value, null, 'cookie removed');
   +      })
   

   This seems odd to me. Shouldn't this be another .execute(getJqueryCookie call?

   I don't think the Nightwatch getCookie method will work here. This might be the reason the test is failing.

1. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,256 @@
   +              .setCookie({
   +                name: 'js_cookie_test_first',
   +                value: 'red panda',
   +              })
   +              .setCookie({
   +                name: 'js_cookie_test_second',
   +                value: 'second red panda',
   +              })
   +              .setCookie({
   +                name: 'js_cookie_test_third',
   +                value: 'third red panda id bad%',
   +              })
   ...
   +              .setCookie({
   +                name: 'SIMPLETEST_USER_AGENT',
   +                value: simpletestCookieValue,
   +              });
   

   These cookies are leaking in the test because this is all in the same browser session.

   The "js_cookie_test_remove" cookie in the failing test is removed, but these other cookies still exist.

   The SIMPLETEST_USER_AGENT cookie probably should remain.

2. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,256 @@
   +      .getCookie('js_cookie_test_remove', result => {
   +        browser.assert.equal(result.value, null, 'cookie removed');
   +      })
   

   So I think instead this should be

         .execute(getJqueryCookie, ['js_cookie_remove_test'], result => {
           browser.assert.equal(result.value, null, 'cookie removed ' + JSON.stringify(result.value));
         })
   

seems we have to skip this test case https://github.com/carhartl/jquery-cookie/blob/master/test/tests.js#L151

test('Call to read all when there are no cookies at all', function () {
	deepEqual($.cookie(), {}, 'returns empty object');
});

+1 to #71.

I moved the remove cookie test to the first test as a clean up, and made the code standards fixes.

Also I found that the non-es6 shim file needed an update.

Should this issue be critical to match its parent?

+++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
@@ -37,6 +38,10 @@
+        browser.assert.equal(result.value, null, 'cookie removed');        ¶

Whoops.

Yay, we're green again! :-)

I updated the issue summary's remaining tasks

Bumping priority based on parent issue being critical.

The test coverage looks much better now which should give us some confidence as we move forward. 👍

1. +++ b/core/core.libraries.yml
   @@ -332,16 +332,13 @@ jquery:
    jquery.cookie:
   

   Should we also deprecate this library?

2. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,194 @@
   +  const deprecatedMessage = `This function is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library directly instead.`;
   

   1. Since we're deprecating this, we have to ensure we don't have any usages of this code in core.
   2. Rather than saying "this function", it would be clearer if we specified which function has been deprecated. This way it's easier to work based on the message especially when the message is presented out of the context. We should also add deprecation messages in the JSDoc format so that IDEs recognize these functions as deprecated.

3. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,194 @@
   +  /**
   +   * Wraps the reading and writing of cookie values.
   +   *
   +   * @callback reader~converterCallback
   +   * @param {string} value
   +   *   The cookie value.
   +   * @param {string} name
   +   *   The cookie name.
   +   */
   +
   

   We can probably remove this documentation?

4. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,194 @@
   +      const attributes = Object.assign($.cookie.defaults, options);
   ...
   +  $.cookie.defaults = Object.assign({ path: '' }, cookies.defaults);
   ...
   +    cookies.remove(key, Object.assign($.cookie.defaults, options));
   

   Object.assign isn't properly supported by IE 11 and Opera Mini. We should provide a polyfill for that. We already have a polyfill for this in #3108402: Update to Popper.js to 2.0.0 so we could copy it here too.

5. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,247 @@
   +        browser.assert.equal(
   

   Should we use strictEqual here?

This addresses 1,2,3,5 of #77

Item 4 is unnecessary as #3108402: Update to Popper.js to 2.0.0 landed which provides the polyfill.

We still have to add the polyfill as a dependency to ensure that it's loaded 😇

We still have to add the polyfill as a dependency to ensure that it's loaded

🤦‍♂️oh of course!

Awesome! Thank you.

I reviewed the patch in #80 and it resolves the code review items in #77. js-cookie now has the polyfill as a dependency, the deprecated messages are more specific, and the extraneous comment block has been removed.

I added a release notes snippet for review.

Adjusted the snippet to be clear that there is a new core js library.

Adding the deprecation to core/jquery.cookie resulted in the many test failures in #80.

Added this to the skipped deprecations list as core/drupal.form and core/drupal.tabledrag still declare core/jquery.cookie as a dependency. They don't actually use the library, but removing it would impact BC for contrib/custom projects that expect cookie to be available when form/tabledrag are used.

Added a comment to each library declaring core/jquery.cookie as a dependency to make it clear this dependency can be safely removed.

+++ b/core/core.libraries.yml
@@ -210,8 +210,11 @@ drupal.form:
     - core/jquery.once
+    # This library makes no use of jquery.cookie, which is deprecated in Drupal
+    # 9.0.0 and removed in Drupal 10.0.0. It remains here to support contrib
+    # projects that expect its presence.
+    - core/jquery.cookie

I don't think we need to do this - contrib projects should be setting their own dependencies, the presence or not of a particular library on a page isn't an API we need to provide bc for.

I don't think we need to do this - contrib projects should be setting their own dependencies, the presence or not of a particular library on a page isn't an API we need to provide bc for.

I think our concern is that a contrib or custom/private module/theme may not even know that they need to explicitly depend on jquery.cookie because their use of this library gives them access to it.

@mradcliffe yeah that's really not something we need to provide bc for.

See also #3113400: Deprecate more jQuery UI library definitions

Okay, so if we don't need to keep jquery.cookie as a dependency where it's not in-use, then we can remove those entries like in #50, but added back in #52, @catch?

1. +++ b/core/core.libraries.yml
   @@ -210,8 +210,11 @@ drupal.form:
   +    # This library makes no use of jquery.cookie, which is deprecated in Drupal
   +    # 9.0.0 and removed in Drupal 10.0.0. It remains here to support contrib
   +    # projects that expect its presence.
   +    - core/jquery.cookie
   
   @@ -275,6 +278,9 @@ drupal.tabledrag:
   +    # This library makes no use of jquery.cookie, which is deprecated in Drupal
   +    # 9.0.0 and removed in Drupal 10.0.0. It remains here to support contrib
   +    # projects that expect its presence.
        - core/jquery.cookie
   
   @@ -406,6 +410,9 @@ jquery.joyride:
   +    # Joyride can function without jquery.cookie, so this dependency can be
   +    # safely removed when core/jquery.cookie is removed in Drupal 10.
   +    # @see https://github.com/zurb/joyride/blob/v2.1.0/jquery.joyride-2.1.js#L32
        - core/jquery.cookie
   

   So we can remove not only the comments but the libraries here because these are not used in core.

2. +++ b/core/modules/system/tests/modules/js_cookie_test/js_cookie_test.libraries.yml
   @@ -0,0 +1,9 @@
   +    - core/jquery.cookie
   

   And the Nigtwatch tests should still pass because jquery.cookie is added as a dependency for the test module's library.

@mradcliffe yes #50 looks more like I was expecting.

Glad to get some clarification that it's acceptable to remove these dependencies - I wasn't 100% sure and this makes for a cleaner core.libraries.yml.

The one part I wasn't sure about was the joyride dependency -- it doesn't need jQuery cookie, but it will use it when available. Removing jquery.cookie could result in an unexpected functionality change. Because I'm not certain how critical it is to continue including the jquery.cookie library with Joyride, I provided two patches, one that removes the library from Joyride (and as a result doesn't need to modify the deprecation skip list), and one that keeps it there.

I would consider making jquery.cookie or our bc layer for it a direct dependency of the jquery.joyride library definition - that would maintain the same behaviour without requiring us to add anything to the skip list.

Looks like there's an open and active issue for replacing jquery.joyride.

#91 sounds good

Diffing from the version that still included core/jquery.cookie with Joyride.

I updated the change record based on #91 and #92.

No comment on the JavaScript but the library deprecation stuff all looks good to me now.

I would consider making jquery.cookie or our bc layer for it a direct dependency of the jquery.joyride library definition - that would maintain the same behaviour without requiring us to add anything to the skip list.

It doesn't seem like we have any test failures with the current code but we are throwing a deprecation warning in the JavaScript code as well. That would lead into deprecation warnings on a standard Drupal installation (assuming JavaScript deprecation warnings have been turned on). Are we okay with that?

@lauriii what if we made jquery.cookie the direct dependency instead of the bc layer? Then it wouldn't throw any deprecations.

@catch Unfortunately, that wouldn't work because they are both loaded globally to the same interface $.cookie.

So I would lean towards js deprecation errors being an acceptable trade-off for not having stuff in the skip-list, but I am not really sure either way to be honest.

Let's also add the dependency evaluation for the new dependency: https://www.drupal.org/core/dependencies#criteria

I tried reviewing the patch WRT the JavaScript deprecation but couldn't tell what the JS deprecation error that would be thrown is.

• Are they Drupal.deprecationError as per https://www.drupal.org/core/deprecation#javascript ?
• Why is it a tradeoff between JS and PHP deprecations?
• Are they thrown on D9 as well as D8?
• If so, do we have any message for skipping an expected deprecation for JS, or a potential followup that would make core not rely on the deprecated code?

Why is it a tradeoff between JS and PHP deprecations?

It's not, that's a mis-speak from me. The current patch has no PHP deprecation because jquery.joyride is updated to use our jquery cookie bc layer directly (i.e. not via the deprecated jquery.cookie library definition). I was thinking if jquery.joyride used the actual jquery cookie js instead of a shim, but you can't use both, and it's being removed from core here.

So the choices for joyride are:
1. PHP and js deprecation errors
2. Only js deprecation errors (current patch)
3. Whatever joyride does for a fallback when jquery.cookie isn't available, no deprecation errors but would need manual testing or similar.

Are they thrown on D9 as well as D8?

Yes it's a bc layer we're adding in 9.x, for removal in 10.x. But we can only remove it in 10.x if we do #3051766: Deprecate and replace jQuery Joyride (for tours), or option #3 above.

+++ b/core/misc/jquery.cookie.shim.es6.js
@@ -0,0 +1,190 @@
+   */
+  $.cookie = (key, value = undefined, options = undefined) => {
+    Drupal.deprecationError({ message: `jQuery.cookie() ${deprecatedMessageSuffix}` });

This is the deprecation that gets triggered.

Isn't it a premature deprecation then, that belongs in the other issue?

It's a bit borderline.

No core JavaScript triggers the deprecation, and we'd want to inform contributed modules to use the new cookie API. In that sense the deprecation isn't premature.

But joyride we can't fix the deprecation in, we can only replace it entirely.

We definitely could remove the deprecation message from the bc layer, then add it back once joyride has been replaced, again for removal in 10.x

We definitely could remove the deprecation message from the bc layer, then add it back once joyride has been replaced, again for removal in 10.x

This seems like the right approac to me. For now we could add a @todo.

OK moving to needs work for that.

@bnjmnm is actively working on this now.

Replaced the messages in the shim with a @todo, will be adding the dependency evaluation next.

1. +++ b/core/core.libraries.yml
   @@ -351,16 +349,14 @@ jquery:
   +  deprecated: The core/jquery.cookie asset library is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library instead. See https://www.drupal.org/node/3104677
   

   This is fine, right?

2. +++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
   @@ -0,0 +1,247 @@
   +const deprecatedMessageSuffix = `is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library instead. See https://www.drupal.org/node/3104677`;
   ...
   +      .assert.deprecationErrorExists(`jQuery.cookie() ${deprecatedMessageSuffix}`)
   ...
   +      .assert.deprecationErrorExists(`jQuery.removeCookie() ${deprecatedMessageSuffix}`)
   

   I think these tests will fail now because t hese should be removed too?

This is a draft of the dependency evaluation. I'm particularly unsure about #2 so I'm not adding it directly to the issue summary. It may require a formal inquiry like this one, which may be best delegated to someone who speaks security.
Here's the draft:

1. Maintainership of the package: Maintained by the excellentcarhartl, who also maintained jQuery cookie. It is actively maintained and the issue queue is very clean. I reviewed closed issues over the past several months and the response time is quite fast
2. Security policies of the package: A documented security policy is not available online. Since this library is essentially a successor to jquery.cookie, the security approval granted to that library may extend to this one, but this is a judgement best suited to someone from the security team. In particular I'm not sure if 2.x will be supported once 3.x is out of beta.
3. Expected release and support cycles: The release schedule is irregular based on the maintainer's availability and need, but there tend to be a few releases a year. are . https://github.com/js-cookie/js-cookie/releases The maintainer follows semver strictly insofar as one can with a JavaScript library. Dots are used in tags for pre-release versions which differs from Drupal but is valid semver.
4. Code quality: > 2800 dependents, available in all popular package managers. It's trusted by many and any concerns will likely be assuaged by quickly reviewing the 163 lines (including whitespace) .
5. Other dependencies it would add, if any: no dependencies, only dev dependencies that Drupal never pulls in.

For the security policies, I would indeed post a question in their queue. We can post an issue like https://github.com/popperjs/popper-core/issues/823 (and additionally, close with a suggestion that they add documentation of the security policy at https://github.com/js-cookie/js-cookie/security/policy).

That doesn't necessarily need to block this issue going in since it's sort of an upgraded version of a dependency we already have, but we can at least start a discussion.

Changing status to Needs work based on Nightwatch test failure. There seem to be some random failures in HEAD too?

Yes, WidgetUploadTest and QuickEditIntegrationTest both have known random fails, so we can ignore that part for now.

Here's the Nightwatch output:

Host commands: sudo -u www-data cp /var/lib/drupalci/workdir/nightwatchjs/.env /var/www/html/core/.env
Return code: 0
Output: 
StdErr: 
Host commands: sudo BABEL_DISABLE_CACHE=1 -u www-data yarn --cwd /var/www/html/core test:nightwatch
Return code: 5
Output: yarn run v1.21.1
$ cross-env BABEL_ENV=development node -r dotenv-safe/config -r @babel/register ./node_modules/.bin/nightwatch --config ./tests/Drupal/Nightwatch/nightwatch.conf.js

[0;36m[Example Test] Test Suite[0m
[0;35m=========================[0m
  Using: [1;34mchrome[0m [0;33m(74.0.3729.157)[0m on [0;36mLinux[0m platform.

Running:  [0;32mTest page[0m

[0;32m✔[0m Element <body> was visible after 31 milliseconds.
[0;32m✔[0m Testing if element [0;33m<body>[0m contains text [0;33m'Test page text'[0m [0;90m(34ms)[0m

[0;32mOK.[0m [0;32m2[0m assertions passed. (299ms)
Running:  [0;32mPage objects test page[0m

[0;32m✔[0m Element <body> was visible after 23 milliseconds.
[0;32m✔[0m Testing if element [0;33m<Element [name=@body]>[0m contains text [0;33m'Test page text'[0m [0;90m(38ms)[0m
[0;32m✔[0m Ensuring no deprecation errors have been triggered [0;90m(19ms)[0m

[0;32mOK.[0m [0;32m3[0m assertions passed. (161ms)

[0;36m[Install Profile Test] Test Suite[0m
[0;35m=================================[0m
  Using: [1;34mchrome[0m [0;33m(74.0.3729.157)[0m on [0;36mLinux[0m platform.

Running:  [0;32mTest umami profile[0m

[0;32m✔[0m Element <body> was visible after 30 milliseconds.
[0;32m✔[0m Testing if element [0;33m<#block-umami-branding>[0m is present [0;90m(16ms)[0m

[0;32mOK.[0m [0;32m2[0m assertions passed. (1.224s)

[0;36m[Js Cookie Test] Test Suite[0m
[0;35m===========================[0m
  Using: [1;34mchrome[0m [0;33m(74.0.3729.157)[0m on [0;36mLinux[0m platform.

[0;32m✔[0m Element <input[name="modules[js_cookie_test][enable]"]> was visible after 538 milliseconds.
Running:  [0;32mTest jquery.cookie Shim Simple Value and jquery.removeCookie[0m

[0;32m✔[0m Element <.js_cookie_test_add_button> was visible after 28 milliseconds.
[0;32m✔[0m Passed [equal]: $.cookie returns cookie value
[0;32m✔[0m Element <.js_cookie_test_remove_button> was visible after 27 milliseconds.
[0;32m✔[0m Passed [equal]: cookie removed

[0;32mOK.[0m [0;32m4[0m assertions passed. (247ms)
Running:  [0;32mTest jquery.cookie Shim Empty Value[0m

[0;32m✔[0m Passed [equal]: $.cookie returns empty cookie value
[0;32m✔[0m Passed [equal]: Cookie value is empty.

[0;32mOK.[0m [0;32m2[0m assertions passed. (23ms)
Running:  [0;32mTest jquery.cookie Shim Undefined[0m

[0;32m✔[0m Passed [equal]: $.cookie returns undefined cookie value

[0;32mOK.[0m [0;32m1[0m assertions passed. (18ms)
Running:  [0;32mTest jquery.cookie Shim Decode[0m

[0;32m✔[0m Passed [equal]: $.cookie returns decoded cookie value
[0;32m✔[0m Passed [equal]: $.cookie returns decoded plus to space in cookie value

[0;32mOK.[0m [0;32m2[0m assertions passed. (30ms)
Running:  [0;32mTest jquery.cookie Shim With raw[0m

[0;32m✔[0m Element <.js_cookie_test_add_raw_button> was visible after 28 milliseconds.
[0;32m✔[0m Passed [equal]: $.cookie returns raw cookie value

[0;32mOK.[0m [0;32m2[0m assertions passed. (162ms)
Running:  [0;32mTest jquery.cookie Shim With JSON[0m

[0;32m✔[0m Element <.js_cookie_test_add_json_button> was visible after 28 milliseconds.
[0;32m✔[0m Passed [deepEqual]: Stringified JSON is returned as JSON.
[0;32m✔[0m Passed [equal]: Cookie value is encoded backwards-compatible with jquery.cookie.
[0;32m✔[0m Passed [equal]: $.cookie returns simple cookie value with JSON enabled
[0;32m✔[0m Element <.js_cookie_test_add_json_string_button> was visible after 26 milliseconds.
[0;32m✔[0m Passed [deepEqual]: JSON used without json option is return as a string.
[0;32m✔[0m Passed [equal]: Cookie value is encoded backwards-compatible with jquery.cookie.

[0;32mOK.[0m [0;32m7[0m assertions passed. (246ms)
Running:  [0;32mTest jquery.cookie Shim invalid URL encoding[0m

[0;32m✔[0m Passed [equal]: $.cookie won`t throw exception, returns undefined

[0;32mOK.[0m [0;32m1[0m assertions passed. (17ms)
Running:  [0;32mTest jquery.cookie Shim Read all when there are cookies or return empty object[0m

[0;32m✔[0m Passed [deepEqual]: $.cookie() returns empty object
[0;32m✔[0m Passed [deepEqual]: $.cookie() returns object containing all cookies

[0;32mOK.[0m [0;32m2[0m assertions passed. (350ms)
Running:  [0;32mTest jquery.cookie Shim $.cookie deprecation message[0m

[0;32m✔[0m Element <.js_cookie_test_add_button> was visible after 24 milliseconds.
[0;32m✖[0m Testing if "jQuery.cookie() is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library instead. See https://www.drupal.org/node/3104677" deprecation error has been triggered in 5000ms - expected [0;32m"jQuery.cookie() is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library instead. See https://www.drupal.org/node/3104677"[0m but got: [0;31m""[0m [0;90m(5082ms)[0m
[0;90m    at Object.Test jquery.cookie Shim $.cookie deprecation message (/var/www/html/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js:236:15)
    at <anonymous>[0m 


[0;31mFAILED:[0m [0;31m1[0m assertions failed and  [0;32m1[0m passed (5.219s)

[0;36m[Js Deprecation Test] Test Suite[0m
[0;35m================================[0m
  Using: [1;34mchrome[0m [0;33m(74.0.3729.157)[0m on [0;36mLinux[0m platform.

[0;32m✔[0m Element <input[name="modules[js_deprecation_test][enable]"]> was visible after 539 milliseconds.
Running:  [0;32mTest JavaScript deprecations[0m

[0;32m✔[0m Element <body> was visible after 27 milliseconds.
[0;32m✔[0m Testing if element [0;33m<h1>[0m contains text [0;33m'JsDeprecationTest'[0m [0;90m(33ms)[0m
[0;32m✔[0m Testing if "This function is deprecated for testing purposes." deprecation error has been triggered [0;90m(6ms)[0m
[0;32m✔[0m Testing if "This property is deprecated for testing purposes." deprecation error has been triggered [0;90m(6ms)[0m

[0;32mOK.[0m [0;32m4[0m assertions passed. (150ms)

[0;36m[Langcode Test] Test Suite[0m
[0;35m==========================[0m
  Using: [1;34mchrome[0m [0;33m(74.0.3729.157)[0m on [0;36mLinux[0m platform.

Running:  [0;32mTest page with langcode[0m

[0;32m✔[0m Testing if attribute [0;33m'lang'[0m of element [0;33m<html>[0m equals [0;33m'fr'[0m [0;90m(25ms)[0m

[0;32mOK.[0m [0;32m1[0m assertions passed. (279ms)

[0;36m[Login Test] Test Suite[0m
[0;35m=======================[0m
  Using: [1;34mchrome[0m [0;33m(74.0.3729.157)[0m on [0;36mLinux[0m platform.

Running:  [0;32mTest login[0m

[0;32m✔[0m Expected element <.user-role-form .machine-name-value> to be visible in 2000ms[0;90m (540ms)[0m
[0;32m✔[0m User "user" was created successfully [0;90m(35ms)[0m
[0;32m✔[0m Passed [equal]: The user "user" was logged in.
[0;32m✔[0m Element <body> was visible after 27 milliseconds.
[0;32m✔[0m Testing if element [0;33m<h1>[0m contains text [0;33m'Reports'[0m [0;90m(31ms)[0m
[0;32m✔[0m Ensuring no deprecation errors have been triggered [0;90m(7ms)[0m

[0;32mOK.[0m [0;32m6[0m assertions passed. (4.047s)

[0;36m[States Test] Test Suite[0m
[0;35m========================[0m
  Using: [1;34mchrome[0m [0;33m(74.0.3729.157)[0m on [0;36mLinux[0m platform.

[0;32m✔[0m Element <input[name="modules[form_test][enable]"]> was visible after 542 milliseconds.
Running:  [0;32mTest form with state API[0m

[0;32m✔[0m Element <body> was visible after 26 milliseconds.
[0;32m✔[0m Element <input[name="textfield"]> was not visible after 25 milliseconds.
[0;32m✔[0m Ensuring no deprecation errors have been triggered [0;90m(18ms)[0m

[0;32mOK.[0m [0;32m3[0m assertions passed. (146ms)
[1;31m_________________________________________________[0m

[1;31mTEST FAILURE:[0m  [0;31m1[0m assertions failed, [0;32m46[0m passed [0;90m(1m 45s)[0m

[0;31m ✖ jsCookieTest[0m
 – [0;90mTest jquery.cookie Shim $.cookie deprecation message[0m [1;33m(5.219s)[0m
[0;36m   SKIPPED:[0m
   - Test jquery.cookie Shim $.removeCookie deprecation message

info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
StdErr: - [0;36mConnecting to chromedriver-jenkins-drupal-patches-36116 on port 9515...
[0m
ℹ Connected to [0;90mchromedriver-jenkins-drupal-patches-36116[0m on port [0;90m9515[0m [0;90m(158ms)[0m.
- [0;36mConnecting to chromedriver-jenkins-drupal-patches-36116 on port 9515...
[0m
ℹ Connected to [0;90mchromedriver-jenkins-drupal-patches-36116[0m on port [0;90m9515[0m [0;90m(195ms)[0m.
- [0;36mConnecting to chromedriver-jenkins-drupal-patches-36116 on port 9515...
[0m
ℹ Connected to [0;90mchromedriver-jenkins-drupal-patches-36116[0m on port [0;90m9515[0m [0;90m(119ms)[0m.
- [0;36mConnecting to chromedriver-jenkins-drupal-patches-36116 on port 9515...
[0m
ℹ Connected to [0;90mchromedriver-jenkins-drupal-patches-36116[0m on port [0;90m9515[0m [0;90m(187ms)[0m.
- [0;36mConnecting to chromedriver-jenkins-drupal-patches-36116 on port 9515...
[0m
ℹ Connected to [0;90mchromedriver-jenkins-drupal-patches-36116[0m on port [0;90m9515[0m [0;90m(220ms)[0m.
- [0;36mConnecting to chromedriver-jenkins-drupal-patches-36116 on port 9515...
[0m
ℹ Connected to [0;90mchromedriver-jenkins-drupal-patches-36116[0m on port [0;90m9515[0m [0;90m(177ms)[0m.
- [0;36mConnecting to chromedriver-jenkins-drupal-patches-36116 on port 9515...
[0m
ℹ Connected to [0;90mchromedriver-jenkins-drupal-patches-36116[0m on port [0;90m9515[0m [0;90m(219ms)[0m.
[0;31m   Testing if "jQuery.cookie() is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library instead. See https://www.drupal.org/node/3104677" deprecation error has been triggered in 5000ms - expected [0;32m"jQuery.cookie() is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library instead. See https://www.drupal.org/node/3104677"[0m but got: [0;31m""[0m [0;90m(5082ms)[0m[0m
[0;90m       at Object.Test jquery.cookie Shim $.cookie deprecation message (/var/www/html/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js:236:15)
       at <anonymous>[0m
error Command failed with exit code 5.

Yep, forgot about Nightwatch. This removes the tests looking for the removed deprecation messages.

Adding the draft dependency evaluation from #111 to the IS so we don't lose track of it, but leaving the tag on for posting the issue as mentioned in #112. We can ask them both about their security policies and about how long 2.x will continue to receive security coverage (if at all) after 3.0.

I read through the dependency evaluation, and changed "a year. are . " to be "a year. Releases are available at ".

I think this is probably the most honest version we can do:

1. We've added a new cookie library, which can be used without triggering any deprecations.
2. We've added a shim for bc with the old jquery.cookie API with its own library definition. Modules using the old library definition will get a deprecation notice in PHP.
3. Joyride relies directly on the bc layer shim (not via the library), because this is a third party library we can't update ourselves - only replace (which has an open issue). This doesn't trigger any deprecation errors in either PHP or js - to avoid giving site owners warnings they can't do anything about.

Once Joyride is replaced (or if we decided to let joyride run without cookie support and rely on its fallback behaviour), we can add the JavaScript deprecation in a 9.x minor release for removal in 10.x (or really it would be OK to just remove the library in 10.x with the existing PHP deprecation).

Note that js.cookie just released 3.0.0-beta4, so we should probably open an issue to evaluate whether it's possible to update to 3.0.0 prior to release if a stable version comes out - will depend on the impact of any changes.

Still need the issue asking about security support before we can commit here.

Inquired about documentation of the security policy here https://github.com/js-cookie/js-cookie/issues/614

CC attribution to @wim-leers as it was based on his PopperJs inquiry 🙂

Thanks for opening that. Removing release management tags.

Tagging for followup for js.cookie 3.

Adding to the IS.

Posted #3118726: Upgrade to js.cookie 3.

Updating the release note based on the maintainer's responses so far about their support policies and version 3; see #3118726: Upgrade to js.cookie 3 for the rest.

reviewing

1. I ran yarn prettier and tests/Drupal/Nightwatch/Tests/jsCookieTest.js changed. Should we still run this for all js changes?

   I see // prettier-ignore is used in this file. Should use before this code which changes when it is run?

   const getJqueryCookie = function(cookieName) {
     return undefined !== cookieName
       ? jQuery.cookie(cookieName)
       : jQuery.cookie();
   };
   

   It seems like we could just let prettier change it but not sure
   < /li>

Otherwise it looks good

whoops didn't mean to assign it berdir

#127 is right, this needed a prettier and build:js

diff --git a/core/misc/polyfills/array.find.es6.js b/core/misc/polyfills/array.find.es6.js
index e24f3a3c68..1dc9e3055a 100644
--- a/core/misc/polyfills/array.find.es6.js
+++ b/core/misc/polyfills/array.find.es6.js
...
diff --git a/core/misc/polyfills/object.assign.es6.js b/core/misc/polyfills/object.assign.es6.js
index 399bdb8c35..1dbb8bb5e9 100644
--- a/core/misc/polyfills/object.assign.es6.js
+++ b/core/misc/polyfills/object.assign.es6.js
...
diff --git a/core/misc/polyfills/object.assign.js b/core/misc/polyfills/object.assign.js
index 289c59ad31..265eb629a3 100644
...
diff --git a/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
index 7480f46ab0..f4cdb01011 100644
--- a/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js

These files weren't in the last patch.

I did notice that they were updated when prettier was run but I guess that is an existing issue. I think we just have to run prettier and then restore these.

Polyfills unprettified.

@bnjmnm thanks for the updates. It looks good to me! RTBC, assuming tests pass

I went through js-cookie v3 changes, and I don't think we would need to do much to keep compatible.

Major upcoming changes:

- Path is now settable via the API
- URL-encoded cookie values may change existing cookies.
- getJSON removed
- IE10 support removed
- defaults removed in favor of withAttributes factory method.

I think this would make the jquery.cookie shim more light-weight in v3.

The biggest concerns are getJSON removed, solved by not recommending its usage in the change record for people using Cookies directly. And any encoding changes as that could affect existing cookie data. The latter may necessitate a separate shim.

+++ b/core/misc/jquery.cookie.shim.es6.js
@@ -0,0 +1,192 @@
+  $.cookie.defaults = Object.assign({ path: '' }, cookies.defaults);

I was testing the patch locally and I noticed

TypeError: cookies is undefined

I updated to latest 9.0.x, applied the patch, ran database updates, cleared cache again, and when I went to /admin, the library was loaded, but window.Cookies was undefined.

I didn't try a fresh 9.0.x install, and I'll do that next to make sure it wasn't something in my local environment.

Yes, i can confirm this on a fresh 9.0.x install.

- As a privileged user, go to /admin.

1. +++ b/core/core.libraries.yml
   @@ -351,16 +349,14 @@ jquery:
   +    - core/drupal
   
   @@ -404,9 +400,9 @@ jquery.joyride:
   +    misc/jquery.cookie.shim.js: {}
   ...
   -    - core/jquery.cookie
   

   This is throwing the error because js-cookie isn't a dependency.

2. +++ b/core/modules/system/tests/modules/js_cookie_test/js_cookie_test.libraries.yml
   @@ -0,0 +1,9 @@
   +with_shim_test:
   

   This library is working and not throwing errors. And so Nightwatch is passing.

   I guess a further test could go to /admin, and assert no console errors.

So basically in order to do the suggestion in #91, we also need to add js-cookie and the polyfill. Order might matter there.

If we add js-cookie as a library dependency, that should be loaded before the polyfill, so it ought to work - needs manual testing though and agreed a sniff-test of browsing to admin and no console errors would be a test coverage addition.

I would also be fine with moving #91 to a follow-up if it's actually making things complicated here. We are still working on a similar strategy in #3113400: Deprecate more jQuery UI library definitions but nothing doing that trick has been committed yet.

Assigning to myself to work on fixing #135, this will get tested manually, but adding a test to look for console errors seems out of scope (as helpful as it would be to have it). There's an existing issue to check for console errors in tests: #3091870: Fail Functional Javascript tests that throw Javascript errors. That issue probably deserves a priority boost.

I didn't realise we don't fail on JavaScript errors, bumped #3091870: Fail Functional Javascript tests that throw Javascript errors to major for now. If we don't have test infrastructure for this then agreed it doesn't make sense to block this issue on it and manual testing should be OK (otherwise we'd never make any js changes at all).

+1 for not adding test coverage for JavaScript errors here and bumping the follow-up to major. Replacing the needs tests tag with needs manual testing tag to make sure that the patch gets manually tested with browsers that are targeted by the polyfills.

I manually tested both #136 and #137, but adding a Nightwatch test has proven tricky due the nightwatch testing profile. Tour on its own does not provide any tours so the error comes from another core module with a tour.

Before @laurii's comment, I did add a test so I've attached the patch.

The patch here adds a test that installs tour and tour_test in the Nightwatch test. Logging in as an admin slows the test down so I opted for a non-admin tour page.

The test-only patch here is the entire patch with only the additional test.

Edit: +1 on manually testing on IE.

Updated issue summary with details about manual testing and linking to #3118726: Upgrade to js.cookie 3.

Manually tested #142 on Chrome and IE (so I could test with the polyfille), and there were no problems.

I spotted one small thing:

+++ b/core/core.libraries.yml
@@ -400,6 +400,7 @@
+    assets/vendor/js-cookie/js.cookie.min.js: { }

It would be better to add js-cookie as a library dependency since it's not a deprecated library, and this ensures it is always loaded first.

I almost crossposted with #3 saying that we should depend on the library instead so +1 on that 🤠

I don't have anything against having extra test coverage. Just would prefer the lack of additional test coverage to not delay this issue from getting done on time 😇

Addressed review by @bnjmnm, and added js-cookie as a dependency instead of a file for jquery.joyride.

Looks like the test-only patch is passing though it shouldn't be. If it's fairly clear what is happening it can be fixed in this issue. I think it's also acceptable to remove it since #3091870: Fail Functional Javascript tests that throw Javascript errors would also provide coverage for that error (and has been priority-bumped) and #145 also pointed out that it's not necessary to delay this issue with test coverage.

Agreed. It was failing for me locally, but I guess something else was going on.

I have an hour or so. I'll try to get a patch here in the next few minutes.

Removes the test in this patch.

I added an interdiff between @bnjmnn's work in 131 and 150 as well as an interdiff between 145 and 150.

Unrelated test failure.

interdiff-2550717-131-150.txt looks good to me. I agree that we can wait on #3091870: Fail Functional Javascript tests that throw Javascript errors to add the test coverage.

I just wrote the patch on #3091870. It doesn't currently apply but if we could get it rerolled writing a FunctionalJavascript test method should ver simple. I don't think we would even need a test module. Just enable tour and goto /admin. Not sure about a Nightwatch test but we should be able to prove it with FunctionalJavascript for now.

Waiting on test to RTBC

I chatted with @bnjmnm and he tested #150 on IE11. I think he will confirm here.

I tested 131 and confirm the error and 150 to confirm it doesn't happen, on chrome for Mac

Can confirm #153, I tested #150 in IE11 and there is no error and the polyfill does its job.

Removing Needs manual testing tag.

ok back to RTBC!

Sorry wrong issue, although one tag was nearly applicable so fixing back to that.

1. +++ b/core/core.libraries.yml
   @@ -351,16 +349,14 @@ jquery:
   +  deprecated: The core/jquery.cookie asset library is deprecated in Drupal 9.0.0 and will be removed in Drupal 10.0.0. Use the core/js-cookie library instead. See https://www.drupal.org/node/3104677
   

   Nit: should we use the %library_id% placeholder here?

2. +++ b/core/core.libraries.yml
   @@ -404,9 +400,10 @@ jquery.joyride:
        assets/vendor/jquery-joyride/jquery.joyride-2.1.js: { }
   +    misc/jquery.cookie.shim.js: {}
   

   Shouldn't the jQuery Cookie shim be loaded before joyride?

   Let's also add a comment that clarifies why we're loading the file manually, pointing to both, this issue and the follow-up.

3. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,192 @@
   +   * Gets or sets a JavaScript cookie.
   

   Nit: let's remove the word JavaScript from here since the cookies are not specific to JavaScript even though they are set in JavaScript. If we feel that it would be unclear to reference to just Cookies, we could call them as browser cookies which we're already doing in $.removeCookie documentation.

4. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,192 @@
   +   *   library README.md file for details.
   ...
   +   * @param {Object} options
   +   *   Optional options. See the js-cookie library README.md for more details.
   

   Nit: Could we add a @see link to the docblock that references the README.md?

5. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,192 @@
   +   * @deprecated in Drupal 9.0.0 and is removed from Drupal 10.0.0.
   +   *   Use the core/js-cookie library instead.
   ...
   +   * @deprecated in Drupal 9.0.0 and is removed from Drupal 10.0.0.
   +   *   Use the core/js-cookie library instead.
   

   Should we link to the change record here?

6. +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -0,0 +1,192 @@
   +    // Once Joyride is removed, this function can trigger a deprecation error.
   ...
   +    // Once Joyride is removed, this function can trigger a deprecation error.
   

   Nit: I guess another option would be to make Joyride not depend on jQuery cookie. Could we rephrase this to be a bit more open ended to not close that door?

7. It seems like we don't have any test coverage for cookie expiration. Could we add test coverage for that? Some example test cases here: https://github.com/carhartl/jquery-cookie/blob/master/test/tests.js#L209.

Working on #159

While working on the feedback in #159, I discovered that removing joyride's dependency on jquery.cookie (or the shim) has minimal consequences.

jQuery cookie is used by Joyride for one reason: if a tour configured with {autoStart: true} has been viewed, this value is stored in a cookie so the tour does not autostart on future visits to the page. If jQuery cookie is not available, Joyride falls back to using localStorage - which works but has less longevity than a cookie.

Tours created with the Tour module would not be impacted at all, as the Tour module API does not expose auto-start. The only potential impact is to contrib/custom modules/themes that use the core/jquery.joyride library directly and have configured {autoStart: true}. Those impacted that don't wish to use the localStorage fallback can make jQuery cookie available via a custom library, and functionality will be unchanged.

This means we can reintroduce deprecation messages in the shim.

I thought based on #118 we would continue to have jquery.joyride depend on jquery.cookie.

This addresses all but one feedback item* in #159 other than that superceded by the realization in #161 that joyride did not need to depend on jquery.cookie.

*the one unaddressed item is #159.7 -- expanding tests to cover cookie expiration. My local nightwatch setup is not cooperating and I didn't want my other changes to wait on those as there's a chance another contributor (with working local nightwatch) may be able to take care of that. If someone is available to do that perhaps they can let me know via Drupal Slack so I know I can attend to things other than my local nightwatch.

Re #162

I thought based on #118 we would continue to have jquery.joyride depend on jquery.cookie.

Discussed this with @catch upon discovering just how minimal the impact would be, and he agreed it would be fine to remove the dependency.

@mradcliffe that was before we investigated exactly what joyride's fallback behaviour was and whether it would change behaviour for core or contrib, and it turns out nothing will be affected (and there's a 1-5 line fix for contrib or custom code we can put in the change record in case it turns out someone is relying on the cookie behaviour somewhere).

Ping @bnjmnm on Slack. I have Nightwatch working and an hour so I'll try writing the cookie expiration test.

Thank you for the clarification, @catch.

Added Needs tests tag again.

I added three tests from jquery.cookie:

1. expires option as days from now -> Test jquery.cookie Shim expires option as days from now
2. expires option as fraction of a day -> Test jquery.cookie Shim expires option as fraction of a day
3. expires option as Date instance -> Test jquery.cookie Shim expires option as Date instance

The sevenDaysFromNow variable in the "expires option as days from now" original test is inaccurately labeled because it's incrementing 21 days rather than 7, so I changed it to "daysFromNow".

The third test is failing. I think that the Object.assign polyfill or native function is doing something to the native Date object so that it no longer has the toUTCString method. I don't have any more time to work on it at the moment so uploading a new patch.

 Error while running .executeScript() protocol action: unknown error: f.expires.toUTCString is not a function

✖ Failed [equal]: (should write the cookie string with expires) - expected "c=v; expires=Thu, 19 Mar 2020 17:55:58 GMT" but got: "[object Object]" (undefinedms)
    at NightwatchAPI.<anonymous> (/var/www/html/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js:299:24)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

I also found the following lint error, but I had trouble addressing it:

core/misc/jquery.cookie.shim.es6.js
  160:23  error  Use an object spread instead of `Object.assign` eg: `{ ...foo }`  prefer-object-spread

Assigning { path.: '' } to a separate variable and doing Object.assign(var, cookies.defaults) failed the entire test run.

Back to Needs work for the failing. test.

This is great @mradcliffe! Turns out the test was failing due an issue with the shim, so it did its job quite well. This patch fixes the shim and ran yarn prettier on jsCookieTest

There's an ESLint warning with the patch:

$ yarn run lint:core-js-passing
yarn run v1.21.1
$ node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json .

/Users/lauri.eskola/Projects/drupal/core/misc/jquery.cookie.shim.es6.js
  167:23  error  Use an object spread instead of `Object.assign` eg: `{ ...foo }`  prefer-object-spread

Fixed #170.