Problem/Motivation

Per discussion in #3238507: Drupal 10 JavaScript dependency plan, the js-cookie library is unused in core after refactoring elsewhere. It is however used in multiple contrib modules.

We should deprecate it in 10.1.0 for removal in 11.0.0 to have one less external JavaScript dependency. Not sure if this also needs us to create a contrib module for it, but probably.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

The js-cookie library is unused in core after refactoring. js-cookie has therefore been deprecated as a core dependency and there is no replacement. Contributed modules or custom code relying on this library should declare their own dependency on it.

CommentFileSizeAuthor
#9 interdiff.7-8.txt665 bytesspokje
#9 3296086-8.patch3.21 KBspokje
#8 3296086-7.patch3.11 KBspokje
#8 interdiff.6-7.txt2.5 KBspokje
#6 interdiff.3-6.txt431 bytesspokje
#6 3296086-6.patch453 bytesspokje
#3 3296086.patch388 bytescatch
#2 3296086.patch2.26 KBcatch

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Version: 9.5.x-dev » 10.1.x-dev
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3296086.patch2.26 KB
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3296086.patch388 bytes

err can't deprecate and delete the asset file at the same time...

The last submitted patch, 2: 3296086.patch, failed testing. View results

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: +Needs change record, +Needs release note

Looks good, but (probably?) needs CR and release notes.

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: -Needs change record, -Needs release note
StatusFileSize
new 3296086-6.patch453 bytes
new interdiff.3-6.txt431 bytes

Added CR and added that to the deprecation message.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

As we discussed in slack

https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/tests/Drupal...
https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/tests/Drupal...

May be good examples of testing deprecated libraries.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new interdiff.6-7.txt2.5 KB
new 3296086-7.patch3.11 KB

@smustgrave said:

reviewing https://www.drupal.org/project/drupal/issues/3296086 and will this need a test? Don’t think I’ve seen an example of a test for a library

I answered:

Good question, so if we do some looking around, we can find Nightwatch test for libraries that already had one (https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/tests/Drupal...) and for the mass-jQueryUI deprecation we have/had a KernelTest (See https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/tests/Drupal...).
So let's add a KernelTest (very heavily) inspired on the latter and let the Core Committers decide if it's needed/wanted. If not, throwing away code is always easy, if yes, we might want to add all the deprecated IE-shims to it.

(Slack thread: https://drupal.slack.com/archives/C04CHUX484T/p1671032144226969)

INSTA-EDIT: shakes fist in anger at the far to nimble typer smustgrave

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3296086-8.patch3.21 KB
new interdiff.7-8.txt665 bytes

Another angry fist shake at PHPCS

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: -Needs tests
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Tests look good.

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3296086-8.patch, failed testing. View results

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Reviewed & tested by the community

Random Chrome-Broke-Down JS test failure, back to RTBC and ordered a retest to make sure.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Added a proper release note. It still handwaves a bit how contrib should add JS dependencies, but good enough for now.

  • xjm committed b6133d88 on 10.1.x 
    Issue #3296086 by Spokje, catch, smustgrave, xjm: Deprecate/remove js-...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +10.1.0 release notes

Committed the deprecation to 10.1.x. Thanks! I also published the CR after adding a few words.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I have added a new module https://www.drupal.org/project/js_cookie for this library dependency. I will work on updating the change record as appropriately with before/after instructions.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

It looks to me like we've missed removing core/js-cookie from 11.x so far... what are the steps to get that to happen, or is it too late?