Upgrade to js.cookie 3

It seems like a RC was released earlier today https://github.com/js-cookie/js-cookie/releases/tag/v3.0.0-rc.0

Well, goldangit. This just bubbled to the top of the priority list.

This is a sort-of-beta-blocker-but-not-really-definitely-a-should-have to test with the RC.

We should also add to the release note from the jQuery Cookie issue to mention that we may be doing a major version update.

I did some analysis of this a few days ago. I revisited and updated the issue summary with the major changes.

I updated the change record of js-cookie a day or so ago to mention that it is not recommended to use getJSON() and provided the v3 methods which also work in v2.

The biggest changes will probably affect anyone using js-cookie for edge cases. Standard use of getting and setting cookies shouldn't matter with maybe an exception for how strings are encoded and decoded between versions.

One concern is that withAttributes and withConverter seem to be mutually-exclusive ways of creating an api instance. You can't both have a converter and change attributes. This might make the jquery.cookie shim a bit more complicated to implement because we needed to support backwards-compatibility for someone setting default options and trying to encode/decode the cookie values in the same way that jquery.cookie did so, which requires a converter. We could ask / file a feature request to allow this.

I think, on a second review, nesting calls should allow attributes and converters.

  const api = window.Cookies
    .withAttributes({ path: '/some/ridiculous/path' })
    .withConverter({ read: () => null, write: (...args) => {});

That should first create an instance to write to /some/ridiculous/path, and then create another instance with the converters.

That would still be a breaking change for anyone using js-cookie without the jquery.cookie shim, but the shim can encapsulate all of this.

We could also recommend that contributed modules that want to do these edge cases write a wrapper of their own. This is described on Design Patterns to Use with JavaScript Cookie: Composition over inheritance. That would still mean a breaking change, but it could be dealt with by checking if Cookies.withAttributes is defined, and then use that pattern, or if it's not, use v2 pattern.

This could be a contrib module, and if there is no use case for setting cookies in core anymore, then jquery.cookie and js-cookie could both be removed in favor of a contrib module.

I'm working on this

I upgraded to js-cookie 3 and updated shim to make tests pass. There's one behavior change in js-cookie which I didn't figure out yet how to solve. In js-cookie 3, when all cookies are being retrieved, corrupted cookies are returned as undefined rather than being left out from the return payload.

The issue in #12 may be due to this change in the new js-cookie

function get (key) {
      if (typeof document === 'undefined' || (arguments.length && !key)) {
        return
      }

So get() should only be called with a variable if that variable is defined, otherwise it returns a bunch of nothing

Something like this ought to fix it:

+++ b/core/misc/jquery.cookie.shim.es6.js
@@ -149,10 +149,15 @@
     const cookiesShim = cookies.withConverter((cookieValue, cookieName) =>
       reader(cookieValue, cookieName, userProvidedConverter, $.cookie.raw),
     );
+    if (key !== undefined) {
+      return $.cookie.json === true
+        ? cookiesShim.getJSON(key)
+        : cookiesShim.get(key);
+    }
 
     return $.cookie.json === true
-      ? cookiesShim.getJSON(key)
-      : cookiesShim.get(key);
+      ? cookiesShim.getJSON()
+      : cookiesShim.get();
   };
 
   /**

This will need a release note snippet before being committed.

Ignore #14 - that wasn't right.

I did some debugging and narrowed it down. I'm not sure why it's happening, but I'll document here and the answer may come to me if I step away for a bit.

The "Test jquery.cookie Shim Read all when there are cookies or return empty object" test passes locally but not on drupal's testbot (including the vagrant build I have on my machine).

The assertion that fails is checking the result of getJqueryCookie

.execute(getJqueryCookie, [], result => {
                browser.assert.deepEqual(
                  result.value,
                  {
                    js_cookie_test_first: 'red panda',
                    js_cookie_test_second: 'second red panda',
                    js_cookie_test_third: undefined,
                  },
                  '$.cookie() returns object containing all cookies',
                );
              })

I refactored getJqueryCookie so it logged the return value before returning it

const getJqueryCookie = function(cookieName) {
  const theJqueryCookie = undefined !== cookieName ? jQuery.cookie(cookieName) : jQuery.cookie();
  console.warn('returned from getJqueryCookie: ' + JSON.stringify(theJqueryCookie));
  return theJqueryCookie;
};

{
    js_cookie_test_first: 'red panda',
    js_cookie_test_second: 'second red panda',
    js_cookie_test_third: null
  }

{ js_cookie_test_first: 'red panda',
     js_cookie_test_second: 'second red panda' }

At first glance it seems like the local test shouldn't be passing, but js_cookie_test_third manages to sneak its way into the results.

Some additional info:
I ran jsCookieTest using jquery.cookie instead of the shim. Since the goal of the shim is to reproduce the behavior of jquery.cookie, I wanted to confirm how it handled bad cookies.

The result returned by no-shim old-style jquery.cookie is

{ js_cookie_test_first: 'red panda',
     js_cookie_test_second: 'second red panda' }

And fortunately this is consistent on drupalci and local. This return value differers from what is being tested for.

+++ b/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js
@@ -222,6 +222,7 @@ module.exports = {
                   {
                     js_cookie_test_first: 'red panda',
                     js_cookie_test_second: 'second red panda',
+                    js_cookie_test_third: undefined,
                   },
                   '$.cookie() returns object containing all cookies',
                 );

The patch adds for js_cookie_test_third: undefined to the object match, but it looks like jquery.cookie would not have returned this. This means that the patch, when run on drupalci, still matches the behavior of jquery.cookie - the test fails because it's looking for a third cookie that shouldn't be there.

Not sure why js_cookie_test_third makes it through locally. At first glance it seems like theres a difference regarding how the return value is handled when the shim's parseCookieValue () hits catch(e){}.

While running jsCookieTest with jquery.cookie, I also encountered this failure

✖ jsCookieTest
 – Test jquery.cookie Shim With raw (309ms)
   Failed [equal]: ($.cookie returns raw cookie value) - expected "red%20panda" but got: "red panda" (undefinedms)
       at NightwatchAPI.<anonymous> (/Users/ben.mullins/Sites/d9/core/tests/Drupal/Nightwatch/Tests/jsCookieTest.js:125:24)
       at processTicksAndRejections (internal/process/task_queues.js:94:5)

So something is out of sync here as well?

Thank you for debugging this! This is the line in jQuery cookie that ensured that cookies that couldn't be decoded aren't present in the object when all cookies are being retrieved: https://github.com/carhartl/jquery-cookie/blob/master/src/jquery.cookie..... Looking at js-cookie code, it doesn't seem like the behavior is the same: https://github.com/js-cookie/js-cookie/blob/master/src/api.mjs#L54. I'm still trying to figure out why #12 passed locally but failed on DrupalCI.

Debugged the tests with @bnjmnm and we came with something that should solve #18, and pass both locally and on DrupalCI without making any changes to tests.

Going through the code and release notes there appear to be three changes that needed to be addressed.

1. Release notes:
   

   Removed getJSON(): use Cookies.set('foo', JSON.stringify({ ... })) and JSON.parse(Cookies.get('foo')) instead

   This is now taken care of in reader() and is covered by existing tests

2. Release notes:
   

   withConverter() no longer accepts a function as argument to be turned into a read converter. It is now required to always pass an object with the explicit type(s) of converter(s):

   The use of cookies.withConverter in the shim was factored accordingly

3. The other difference is that when js-cookie retrives all cookies, it will include ones with invalid cookie names. jquery-cookie does not return these particulary cookies,so the shim was modified to match that behavior. I think that can be changed slightly but the approach looks sound

   +++ b/core/misc/jquery.cookie.shim.es6.js
   @@ -146,13 +161,31 @@
   +    return (() => {
   +      if (key === undefined) {
   +        const results = cookiesShim.get();
   +        Object.keys(results).forEach(resultKey => {
   +          if (results[resultKey] === undefined) {
   +            delete results[resultKey];
   +          }
   +        });
   +
   +        return results;
   +      }
   

   Is it necessary to iffe this?

   The forEach can be shortened a bit like

   Object.keys(results).forEach(
           resultKey =>
             results[resultKey] === undefined && delete results[resultKey],
         );
   

Addressed #20.3

Perhaps there's a better status to switch to. This is essentially an RTBC but can't be considered for commit until js-cookie 3 gets a full release - setting to postponed so there's no unnecessary additional work done on this. Unless something major changes between the RC and full, we'll just need a final patch that swaps in the full release.

@catch, @effulgentsia, and I discussed this today and we think we'd actually prefer to depend on their RC than be stuck between an unsupported version and a major BC break during our RC or after. So, setting back to NR and we can RTBC it properly if that's correct. It still needs a release note and whatnot.

I was prepared to RTBC this in #22 had the full release been available. Now that it's decided we can use RC0, this is good to go. (having followed the Github activity in js-cookie for the past few months, I'm pretty comfortable with using rc0)

Added a release note snippet for updating from 9-beta1 and another for all other update situations.

Reviewing now!

I attached https://www.drupal.org/node/3104677 to this issue. I think it needs to be updated?

Are there other CRs we need to update? Looking at #7.

I updated https://www.drupal.org/node/3104677 to reflect the change to v3, this included editing/removing several items specific to 2.x or the fact that 3.x was imminent.

Regarding the other CRs possibly needing updating question: I reviewed #7 and the similar comments following. This should not require any additional CR additions/changes. It was possible to adapt the shim to js-cookie 3.0.0-rc0 without having to compromise any functionality (comment #8 suspected this was possible...) . The shim being undisrupted is also confirmed by the fact that the (pretty thorough) test coverage did not have to change.

(rtbc since the CR is now updated)

I tweaked the CR a bit to bring back the notes on differences between v2 and v3 (for anyone who tested the beta). Also adding a link to the CR for the release notes.

Alright, reviewed all the changes here as well as all the comments. Looking good! I don't totally understand the test changes, but I understand why the were needed based on the issue discussion, and given both @bnjmnm and @lauriii were +1 on them (plus that, you know, they're passing) I'll go ahead and commit this.

We'll want to create a followup to update to 3.0.0 once it's released.

Saving issue credit...

I tried to commit this and got a transpilation error on #21:

yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js
[16:35:00] '/Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js' is being checked.
[16:35:01] '/Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js' is not updated.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js
[16:35:03] '/Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js' is being checked.
[16:35:04] '/Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js' is not updated.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Did we forget to re-transpile after fixing the tests?

We also need to update the library in 8.9.x where we provide it for forward-compatibility. Will the same patch work for 8.9.x?

Looks like forgetting to run build:js, a patch for 9.0.x

Failed applying the patch in #35 to 8.9.x. I am not good at es6, so adding a needs reroll tag instead,

Verified that #35 passes on the yarn build:

yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js
[06:43:37] '/Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js' is being checked.
✨  Done in 0.77s.
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js
[06:43:39] '/Users/xjm/git/maintainer/core/misc/jquery.cookie.shim.es6.js' is being checked.
✨  Done in 0.64s.

There are two ways we can indicate on an issue needs the patch backported. The best way is to set the branch to the lowest version needed. If the lower version's patch is not available at the time of commit, the committer will first commit it to the higher branches and then set the status to "Patch (to be ported)". We also sometimes use "Needs backport to X" issue tags, but the branch selector is preferable.

Thanks @jungle!

Thanks all, here's a reroll for 8.9.

Re #32, thanks @xjm!

+++ ./3118726-8.9.x-37.patch	2020-04-29 14:14:13.274155801 +0100
@@ -186,14 +186,18 @@
+ ¶

@@ -202,11 +206,12 @@
+ ¶

@@ -229,8 +234,9 @@
+ ¶

Just FYI. The interdiff in #38 looks like one of the latest patches added unexpected leading space, or removed leading space, but checked the two patches on my local, nothing found.

Thanks!

Followup created #3132164: Update to js-cookie 3.0.0 when it is released

The 9.x patch in #35 addresses the lack of transpiling.
The 8.x patch in #38 brings in the changes from the 9.x patch correctly. I suspect the spaces mentioned in #39 are due to how the interdiff was generated. It was against two patches vs. against core, which we are more accustomed to, but in this case was helpful to have been done this way.

However, I spotted something that needs to be changed in both patches. Both have two instances of
@see https://github.com/js-cookie/js-cookie/blob/v2.2.1/README.md which references the README for the version being replaced. The contents of the respective readmes are significant enough that this should be updated to https://github.com/js-cookie/js-cookie/blob/v3.0.0-rc.0/README.md

Thanks @jungle for carefully looking over the reroll diff, they're always a pain to parse (:

However, I spotted something that needs to be changed in both patches

Good spot @bnjmnm, thanks, fixed!

#40 addresses the last outstanding item, and I did an additional check of both patches to confirm they are correctly built/linted based on the differences in those tools in 8.x and 9.x

Alright, I carefully reviewed the patches again and confirmed that the change record addresses the API changes for the new version (primarily an added parameter and related internal changes to the encoding functionality). I also compared the diffstats to ensure they're equivalent.

Committed the 9.0.x patch to 9.1.x and 9.0.x, and the 8.9.x patch to that branch. The CR is already published from the previous issue. Thanks!