Problem/Motivation

for #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS we need a reliable solution for loading js files and know when the file has finished loading.
loadjs is a working, stable solution to load scripts/css/assets and know when they loaded. It is also tiny: only 899 bytes!

Steps to reproduce

N/A

Proposed resolution

N/A

Dependency evaluation

  • Maintainership of the package: created and maintained by Andres Morey
  • Security policies of the package: none I could find
  • Expected release and support cycles: I don't see any roadmap, last release was 3 years ago. package is feature complete so i wouldn't expect new releases.
  • Code quality: code is easy to read and understand, uses jshint to validate code style, there is a pretty comprehensive test suite
  • Other dependencies: no runtime dependencies, dev dependencies are gulp and jshint

The code is pretty straightforward, we could probably roll our own if necessary, the heavy lifting can be done with something like this. Better to use something battle-tested though, especially when it's small like this.

Remaining tasks

  • patch

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Issue fork drupal-3228351

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_ created an issue. See original summary.

nod_ opened merge request !1064

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
Status: Active » Needs review
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Priority: Normal » Major
Issue summary: View changes
Issue tags: +blocker

Diff looks great — the key remaining thing here is the evaluation of this library 👍

Since this blocks a major, marking as such.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
nod_’s picture

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

added library evaluation

author is working on a dependency manager https://github.com/muicss/johnnydepp that uses the loadjs lib so it's good enough to reuse after several years in another project :)

Phil Wolstenholme’s picture

Phil Wolstenholme CreditAttribution: Phil Wolstenholme commented

This isn't a formal evaluation, but we've (a Drupal agency in the UK) been using LoadJS (via the 'Drupal.ajax does not guarantee that 'add new JS file to page' commands have finished before calling said JS' patch from the previous DO issue) since 8 Feb 2020 and I've grown to really love it.

It fixes the Drupal.ajax issue, which is great, but I've also been using it for some performance enhancements too:

  • Use LoadJS to load things like the Vimeo and YouTube API JS files later on, so they don't need to be included in the page's HTML response and be blocking. I could use `defer` instead, but then adding an attribute to the library prevents Drupal aggregation.
  • Use Loadjs for a sort of basic manual lazyloading/code-splitting. For example, only loading our focus-trapping script when an offcanvas menu or modal is opened. This saves us sending every user the focus-trapping JS even if they will never interact with a page element that needs it.
  • Loading https://github.com/nuxodin/ie11CustomProperties only if the user is using IE11
  • Loading https://markjs.io on search result pages later than other scripts, so this very un-vital JS does not compete with more important JS when it comes to downloading and execution
  • Loading https://github.com/willmcpo/body-scroll-lock only when a menu is opened

All of the above I could have done manually of course, but having LoadJS available in Core (via the patch) made it so easy :)

We use it on sites like https://www.ltmuseum.co.uk that are relatively highly trafficked and we haven't seen any bugs related to LoadJS, so it's a +1 for bringing it into Core from me!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

author is working on a dependency manager https://github.com/muicss/johnnydepp

🤣🤣🤣

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks for adding the library evaluation!

https://github.com/muicss/loadjs/blob/master/src/loadjs.js is 305 lines in total. \Drupal (that is core/lib/Drupal.php) does nothing and has more than twice as many lines.

The tiny size, the low complexity, complete test suite combined with the endorsement in #7 are reason enough for me to mark this RTBC.

I can't wait to see #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS getting unblocked!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs release manager review

Since this adds a new dependency, this needs to be reviewed by a release manager

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Security policies of the package: none I could find

Usually when this is the case we open an issue on github and ask if they have an informal/undocumented policy for what they would do in the case of a vulnerability.

If the answer is 'we don't have one' that's OK, but in the past we've ended up with something basic we could document.

Everything else looks good to me on the dependency evaluation.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

I think we could still open a short issue in their queue asking if they could define a security policy using the GH feature with some of the questions we normally ask, but given that this is a very small amount of code to fork if it should become necessary, I don't think we would need to block committing this on an answer.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: -Needs release manager review

#12 works for me too, removing the needs RM review tag.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented

posted: https://github.com/muicss/loadjs/issues/106

  • catch committed fe2e7c3 on 9.3.x 
    Issue #3228351 by nod_, Phil Wolstenholme: Add loadjs library
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Really nice and encouraging response on github. Let's get this in.

Committed fe2e7c3 and pushed to 9.3.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

As there's no CR maybe release note snippet is needed?

Status: Fixed » Closed (fixed)

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

nod_ closed merge request !1064

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented
Related issues: +#3336143: Uncaught ReferenceError: loadjs is not defined after drupal core upgrade 9.5.1

Follow-up: #3336143: Uncaught ReferenceError: loadjs is not defined after drupal core upgrade 9.5.1