Problem/Motivation

Several of admin_toolbars JS files/behaviors depend on core libraries (e.g. core/drupal, core/jquery, core/once) but do not explicitly declare those dependencies in admin_toolbar.libraries.yml.

This hasn't proven an issue in the past, as they all declare a dependency on admin_toolbar/toolbar.tree, which, in turn depends on toolbar/toolbar (core's Toolbar module), which has ensured that all the necessary libraries were available.

An evaluation of core's approach to dependencies indicates this may not be best practice (what happens if the lib you were depending one to load all those dependencies drops one or more of them?). That suggests that admin_toolbar should explicitly declare any dependencies for each file/behavior.

Proposed resolution

Decide on whether or not to declare all dependencies, evaluate each JS file and make the appropriate changes to admin_toolbar.libraries.yml.

-OR-

Stick with the current approach and close this issue.

Remaining tasks

  • Decide approach
  • Open MR
  • Review and test
  • Merge

User interface changes

None

API changes

None

Data model changes

None

Issue fork admin_toolbar-3529217

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

justcaldwell created an issue. See original summary.

erutan’s picture

erutan commented

Explicitly adding dependencies makes it less likely the module will break in the future, at the cost of having to explicitly declare them once. Unless a dependency name changes while it's functions don't or something kind of bizarre like that there doesn't seem to be much of a downside unless I'm missing something obvious.

It's also a bit self-documenting which is nice.

dydave’s picture

dydave commented
Version: 3.6.1 » 3.x-dev

Thanks a lot Michael (@justcaldwell), once again for creating this issue with a very clear description and for looking into how Drupal core seems to address this currently. 🙏

Let's move this to the top of the stack and try to include this in the next release.

I looked a bit at module's code and it looks like we would need to add the following dependencies:

    - core/jquery
    - core/drupal
    - core/once

For libraries: admin_toolbar/toolbar.tree and admin_toolbar_search/admin_toolbar_search.

I think I've picked up a few other changes and would like to add some inline comments here and there in YAML files, but I should be able to create an initial merge request to be reviewed in the coming days.

I don't really see any blocker or major issue that would prevent us from making this change.

Feel free to let us know if you would see anything else that would need to be addressed in this issue, I would be glad to group the changes at the same time in the merge request.
Coming back to this issue very soon....

Thanks in advance! 😊

dydave opened merge request !200

dydave’s picture

dydave commented
Status: Active » Needs review

Quick follow-up on this issue:

Created initial merge request !200 above at #4 including the changes to the libraries detailed at #3.

Not sure whether the changes to the JS files could potentially fix another issue #3557369: Disabled, show on scroll-up does not work in Git-version...
I picked these up while reviewing the various library dependencies and calls in the JS files.

Moving issue to Needs review, as an attempt to get more testing feedback and code reviews.

Feel free to let us know if you have any questions or concerns on any of the suggested code changes or this issue in general, we would surely be happy to help.
Thanks in advance! 😊