Missing jQuery.once dependencies on some JS files.

Do not register all JS files. for example when we load core/drupal.form, it will also loaded the dependencies (core/jquery & core/drupal) automatically.

Thinking that I will spend some time this week to review all JS dependencies :)

CommentFileSizeAuthor
#12 jquery_once_dependencies-12.patch3.52 KBdroplet
#6 jquery_once_dependencies.patch3.52 KBdroplet
#2 nodejs.patch373 bytesdroplet
nodejs.patch691 bytesdroplet

Comments

nod_’s picture

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

I disagree. Don't remove dependencies like this please.

What happens if I override jquery.once from contrib to remove it's jQuery dependency? everything breaks. If it's explicitly used in the JS file, it needs to be explicitly declared in the dependencies. If I'm using jQuery it needs to be a dependency. Knowing the exact level in which a dependency is needed is required to sort the dependency graph properly.

The name of the library does not suppose there is a hard dependency on anything. Dependencies are declarative, not implied.

droplet’s picture

droplet commented
Status: Needs work » Needs review
StatusFileSize
new nodejs.patch373 bytes

hmm.. Sounds right.

nod_’s picture

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

Thanks. All good :)

droplet’s picture

droplet commented

@nod_,

do you mind to review all changes in one Patch ?? I made it already, just 11 changes. Saving thousand years to create 11 issue threads. :)

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Fix node JS dependencies » Fix JS dependencies
Status: Reviewed & tested by the community » Needs work

Yeah, totally, go for it.

droplet’s picture

droplet commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new jquery_once_dependencies.patch3.52 KB
nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Fix JS dependencies » Fix JS dependencies on jquery.once
Status: Needs review » Reviewed & tested by the community

Manually confirmed for each file the need for jquery.once dependency. All good.

Thanks.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: jquery_once_dependencies.patch, failed testing.

droplet’s picture

droplet commented
Status: Needs work » Needs review

6: jquery_once_dependencies.patch queued for re-testing.

droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community

back to RTBC #7. testbots self errors only.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Sorry, this one needs a re-roll.

droplet’s picture

droplet commented
Status: Needs work » Needs review
StatusFileSize
new jquery_once_dependencies-12.patch3.52 KB

Git rebased. ( no difference)

nod_’s picture

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

thx

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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