Problem/Motivation

PopperJS is deprecated in Drupal 9 as it was only used by Quickedit, a module that is not in Drupal 10:
#3307471: Deprecate popperjs

Proposed resolution

Remove PopperJS in Drupal 10

Remaining tasks

User interface changes

none

API changes

not sure yet.

Release notes snippet

The popperjs library was used only by Quick Edit, which has been removed from core. The popperjs library is therefore also removed, because core is no longer using it. If you need to use it, you should redefine the library in a custom module.

Issue fork drupal-3301545

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nod_ created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Critical
Issue tags: +Drupal 10 beta blocker
Parent issue: » #3118149: [meta] Requirements for tagging Drupal 10.0.0-beta1

Per @nod_, we also need to file an upstream PR to update Shepherd.js for this change.

nod_’s picture

nod_
Primary language French
Location Lille
commented

shepherd upstream issue opened: https://github.com/shipshapecode/shepherd/issues/2013

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 9.5.x-dev » 10.0.x-dev

upstream PR started.

This can only be a 10.x patch since floating UI uses await/async

catch’s picture

catch
he/him
Primary language English
commented

This is referred to in a few tests, but just as example-ish content not actual test coverage. We could switch those to floating-ui or just something else entirely:

core/modules/system/tests/themes/test_theme/test_theme.info.yml
core/modules/system/tests/themes/test_basetheme/test_basetheme.info.yml
core/modules/system/tests/modules/common_test/common_test.module
core/modules/system/tests/modules/common_test/common_test.libraries.yml
core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php

The only other core usage, apart from via Shepherd, was in quickedit, which is removed from Drupal 10, so that leaves us with swapping around the library definition and updating the assets.

Main question for me then is what do we do about removing the popperjs if this is a 10.x only patch, can we:

1. Backport the floating-ui library to 9.5.x only as a library definition, so that it's available to contrib modules (who are entitled to drop IE11 support before 9.5.x is EOL). We've then got the option to deprecate it for removal in Drupal 10 or 11.
2. Deprecate popperjs in 10.0.0 for removal in 11.0.0, with no 9.5 backport.
3. Remove popperjs from 10.0.0 with no deprecation (not really)

#1 seems better for modules dealing with Drupal 10 compatibility.

nod_’s picture

nod_
Primary language French
Location Lille
commented

I'd say 1. as well, but we might not need to add that library to core at all? if we don't have any use for it I don't see the reason to add a unused library to core?

It's not a drop-in replacement so there will be work on their end, might as well let people install popperjs library on their own and keep their code running. I don't see a need to enforce a rewrite of their code.

catch’s picture

catch
he/him
Primary language English
commented

@nod_ don't we need it for Shepherd though?

nod_’s picture

nod_
Primary language French
Location Lille
commented

it's built in the library. when we use shepherd and popperjs in the same page we essentially load popperjs twice today. see https://github.com/shipshapecode/shepherd/issues/1857

nod_’s picture

nod_
Primary language French
Location Lille
commented
Related issues: +#3307471: Deprecate popperjs
catch’s picture

catch
he/him
Primary language English
commented

Ohhhh got it. In that case should we just mark this duplicate of #3307471: Deprecate popperjs?

nod_ opened merge request !2724

nod_’s picture

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

Let's see what happens if we just remove it.

Guess it's a little different between the two branches?

catch’s picture

catch
he/him
Primary language English
commented
Title: Replace popperjs with floating-ui » Remove popperjs from Drupal 10
nod_’s picture

nod_
Primary language French
Location Lille
commented

Strictly speaking we don't need the shepherd PR given that the use of popperjs is their responsibility and it's built-in the library.

It's good collaboration for open source in general though so I'll still try to make it land and update the tour module to work once it's released.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

I think Popper can be taken out of the eslint globals.
That's the only code change I can see being needed. The issue summary also needs updating to reflect the new approach. Popper is a nice library, but it's nice to be able to have fewer dependencies.

Spokje made their first commit to this issue’s fork.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Applied the suggested change by @bnjmnm in #15, back to NR for code review.

Still needs an IS update, but I'm certainly not the JS-guru that can pull that off.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue summary: View changes
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue tags: -Needs issue summary update
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Reviewed & tested by the community

@Spokje addressed my feedback.

Compared this to the changes in #3307471: Deprecate popperjs and the changes that need to be in both issues are nicely consistent with each other. Also pleased that Spokje spotted that popperjs could be removed from spellcheck. I'd assumed the Shephard use would keep that needed, but always nice when the dictionary can be reduced a bit.

  • catch committed 8398ea3 on 10.0.x 
    Issue #3301545 by nod_, Spokje, bnjmnm: Remove popperjs from Drupal 10...
  • catch committed c918ca9 on 10.1.x
    Issue #3301545 by nod_, Spokje, bnjmnm: Remove popperjs from Drupal 10
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Fixed » Needs work
Issue tags: +10.0.0 release notes, +Needs release note

This should probably be in the release notes, since it's a removed/deprecated dependency.

I think the current release note could also be a little more detailed. Re-opening for that.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes

tried to update the release note.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
Status: Needs review » Fixed
Issue tags: -Needs release note

Made a few changes, including an actionable suggestion for people who need the library. ;) Moving this back to fixed. Thanks @nod_!

Status: Fixed » Closed (fixed)

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