Properly declare external dependencies

As long as Drupal core contains these, it seems fine to depend on them?

As long as Drupal core contains these, it seems fine to depend on them?

Sure, but core doesn't contain all of them anymore. Especially Underscore and Backbone, which are heavily used by Quick Edit, but which no longer appear in core.libraries.yml.

The short-term fix would be to change those dependencies to core/internal.backbone and core/internal.underscore, but they are explicitly marked as being liable to disappear at pretty much any time. Quick Edit should probably just redefine the required libraries and depend on them that way.

This issue therefore seems like it might be critical, since the module is already depending on libraries that don't exist except in Drupal 9.

D'oh, I'd forgotten that Underscore and Backbone are gone in 10.0.x! 🙈

Quick Edit should probably just redefine the required libraries and depend on them that way.

Except that shipping those third party libraries directly with a contrib module (i.e. committed to the git repo) is not allowed by the Drupal.org policy: https://www.drupal.org/docs/develop/git/setting-up-git-for-drupal/drupal... 😬

The easiest path forward here is probably due to load them externally: https://www.drupal.org/docs/develop/git/setting-up-git-for-drupal/drupal...

The easiest path forward here is probably due to load them externally

Yeah, that's what I meant by "redefine". Sorry, shoulda said! :)

Self-assigning to implement.

https://git.drupalcode.org/project/quickedit/-/merge_requests/13/diffs?c... → why add the file directly instead of again loading it externally? If merged, this would violate the d.o policy just like #3306909: Get Drupal Security Team approval to commit vendor/ckeditor.js to the drupal/ckeditor project — see https://www.drupal.org/docs/develop/git/setting-up-git-for-drupal/drupal....

But … here's a silver lining: why do we even need this? The reasoning for this dependency can be found in #3244855-10: Regression in Drupal 9.3.x due to jQuery.once deprecation. AFAICT, as long as tests pass without triggering JS deprecation warnings, we can just drop the jquery.once.bc dependency altogether!

AFAICT #3183149: Deprecate jquery.once and use the new once lib updated all of Quick Edit's uses of $.once() to use once() directly. So … we should be able to just delete it altogether. AFAICT this was just a universal solution that was blindly applied in #3244855: Regression in Drupal 9.3.x due to jQuery.once deprecation, but should not have been.

Assigning to @nod_ to review this, because he was closely involved in all of those issues :)

Yes please get rid of jQuery.once.bc, that was added for extra precautions and made sense while this was in core for code that implicitly depended on jQuery.once.

In contrib there is no reason to keep it around. Like Wim said the module already uses once without jQuery.

Cool. But … it looks like something is still causing failures here.

@phenaproxima What's your analysis on the remaining failures? Is it related to Settings Tray?

I don't know. Most of these don't fail for me on my localhost.

Looks like these problems are due to HEAD being broken on 10.0.x: https://www.drupal.org/pift-ci-job/2469714

I suspect that this is due to the removal of Popper from core 10.0.x, which happened earlier today: https://git.drupalcode.org/project/drupal/-/commit/8398ea32a0c2d2006ec49...

Whew. The 10.0.x failure is pre-existing in HEAD: https://www.drupal.org/pift-ci-job/2468637

@xjm informed me that Quick Edit is unlikely to be decoupled from Backbone/Underscore, and will probably just get EOLed. Therefore, it's safe to rely on the internal libraries.

This is ready for review.

We should probably re-scope and re-title this to reflect the revelations from @xjm in #18.

I don't understand why we'd continue to rely on core/internal.* asset libraries?

Quoting core.libraries.yml:

internal.backbone:
  # Internal library. Do not depend on it outside core nor add new core usage.
  # The library will be removed as soon as the following issues are fixed:
  #  - https://www.drupal.org/project/drupal/issues/3203920
  #  - https://www.drupal.org/project/drupal/issues/3204011
  #  - https://www.drupal.org/project/drupal/issues/3204015

That means that at any point in time, i.e. once Contextual Links, Tour and Toolbar stop using Backbone & Underscore, that these libraries will disappear. I think it's safer to bring back the earlier commits that removed these dependencies in favor of locally declared externally loaded libraries?

OTOH … they would result in e.g. window._ being defined twice on some pages. Is that the concern here? I would agree with that. But then I think we should add @todo comments here, to help our future selves.

Discussed this with @lauriii and @nod_:

OTOH … they would result in e.g. window._ being defined twice on some pages. Is that the concern here? I would agree with that. But then I think we should add @todo comments here, to help our future selves.

→ they agree with this. Meaning: A) ship this as-is now because it's the safer solution now, B) update this whenever a 10.x minor removes those core/internal.* libraries to instead use the CDN-based solution that @phenaproxima had in an earlier patch 👍