Problem/Motivation

We use Shepherd.js for rendering Tour. Shepherd.js has a faster release cadence compared to Drupal so it might make sense to explicitly declare Shepherd.js as internal because we will be required to ship major updates within minor releases, and there shouldn't be any strong reason why we should provide Shepherd.js as an API.

Proposed resolution

Deprecate the current Shepherd.js library in Drupal 9.5.x and mark internal in Drupal 10.0.0.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

The Shepherd.js library has been deprecated in Drupal 9.5.0. There is no replacement.

CommentFileSizeAuthor
#11 3308786-11-d10.patch1.31 KBlauriii
#10 interdiff.txt381 byteslauriii
#10 3308786-10-d95.patch2.37 KBlauriii
#8 interdiff.txt567 byteslauriii
#7 3308786-7-d95.patch2.35 KBlauriii
#2 3308786-2-d95.patch2.35 KBlauriii
#2 3308786-d10.patch1.31 KBlauriii

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Needs review
Issue tags: +Needs release note, +Needs change record
StatusFileSize
new 3308786-d10.patch1.31 KB
new 3308786-2-d95.patch2.35 KB
catch’s picture

catch
he/him
Primary language English
commented 
+++ b/core/core.libraries.yml
@@ -900,6 +900,17 @@ shepherd:
+
+internal.shepherd:
+  remote: https://github.com/shipshapecode/shepherd
+  version: "9.1.0"
+  license:
+    name: MIT
+    url: https://raw.githubusercontent.com/shipshapecode/shepherd/v9.1.0/LICENSE
+    gpl-compatible: true
+  js:
+    assets/vendor/shepherd/shepherd.min.js: { minified: true }

Why not move this to tour module itself?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Core has always been responsible for providing library definitions for 3rd party dependencies. I have no idea why that is so but it's just how it's always been. IMO we could move the Shepherd.js and CKEditor 5 library definitions to the respective modules but that would require some updates to the vendor-update script since it can only edit the core.libraries.yml file at the moment.

catch’s picture

catch
he/him
Primary language English
commented

OK that's a good reason not to do it in this issue, we should maybe think about a general follow-up to revisit this if we're going to have lots of internal libraries in core.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work 
+++ b/core/scripts/js/vendor-update.js
@@ -173,9 +173,13 @@ const assetsFolder = `${coreFolder}/assets/vendor`;
+      library: 'shepherd.underscore',

internal.shepherd no?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3308786-7-d95.patch2.35 KB

Seems like this snafu was only in the 9.5.x patch so the 10.0.x patch from #2 is still good.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new interdiff.txt567 bytes
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

D10 patch is good to go, 9.5 still has a easy fix to make

+++ b/core/scripts/js/vendor-update.js
@@ -173,9 +173,13 @@ const assetsFolder = `${coreFolder}/assets/vendor`;
+    {
+      pack: 'shepherd.js',
+    },

This should be 

{
  pack: 'shepherd.js',
  library: 'shepherd'
}

otherwise the library version won't be updated, which is the point of this entry to the vendor-update file :)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3308786-10-d95.patch2.37 KB
new interdiff.txt381 bytes

I guess that was broken before this then given that the library key was not defined there before. 10.0.x patch from #2 is still good.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new 3308786-11-d10.patch1.31 KB

Rerolled D10 patch now that #3308783: Update shepherd.js to 10.0.1 has landed.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

it was working because the folder key was set correctly (see this line in vendor-update.js: const libraryName = library || folder || pack;)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Ah I see! That explains! Thank you @nod_!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
Issue tags: -Needs release note
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: Mark shepherd.js as internal in Drupal 10 » Mark Shepherd.js as internal in Drupal 10
Issue summary: View changes
Issue tags: -Needs change record

The last submitted patch, 10: 3308786-10-d95.patch, failed testing. View results

  • catch committed 72321ad on 10.0.x 
    Issue #3308786 by lauriii, nod_: Mark Shepherd.js as internal in Drupal...
  • catch committed 64ea0be on 10.1.x
    Issue #3308786 by lauriii, nod_: Mark Shepherd.js as internal in Drupal...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release manager review

I double checked contrib, the only references are in install profiles (would be nice if we could filter those out on gitlab) https://git.drupalcode.org/search?group_id=2&scope=blobs&search=core%2Fs...

Given that, while this is a bit close to the wire, I don't think it's going to affect anyone and except for making updates easier.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

  • catch committed 35ba1d5 on 9.5.x 
    Issue #3308786 by lauriii, nod_: Mark Shepherd.js as internal in Drupal...
quietone’s picture

quietone commented

Published the CR.

Status: Fixed » Closed (fixed)

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