Skip to content

Fix inline scripts breaking when using jQuery on empty source scripts#74

Open
WRasada wants to merge 3 commits intomasterfrom
fix/exclude-inline-scripts-from-group
Open

Fix inline scripts breaking when using jQuery on empty source scripts#74
WRasada wants to merge 3 commits intomasterfrom
fix/exclude-inline-scripts-from-group

Conversation

@WRasada
Copy link
Copy Markdown

@WRasada WRasada commented Sep 4, 2023

Expected/Desired Behavior

Scripts registered/enqueued without a source should add inline scripts after jQuery has been loaded.

Actual Behavior

Registering a dummy script without a src and using wp_add_inline_script() adds to DOM before jQuery is loaded.

For example, the plugin PublishPress Capabilities registers a dummy script to attach an inline script using jQuery: https://github.com/publishpress/PublishPress-Capabilities/blob/06000a5be5f896e0f3cdcbe3990afe9310243e90/includes/functions-admin.php#L266-L271

The current logic processes empty src scripts before jQuery is defined. This PR adds an extra check within the first group definition to not process scripts that contain inline content, which is also later checked at line 113:

if ( $this->has_inline_content( $handle ) ) {
$do_concat = false;
}

Steps to Reproduce

Add this code to your theme to register a dummy script with an inline jQuery script:

add_action('wp_enqueue_scripts', 'http_concat_inline_scripts') 

function http_concat_inline_scripts() {
    wp_register_script('dummy-script', false, ['jquery'] );
    wp_enqueue_script('dummy-script', false, ['jquery'] );
    wp_add_inline_script('dummy-script', 'jQuery.ready(function(){console.log("jQuery is defined")});');
};

Load a page on the frontend and check browser dev tools to find the jQuery is not defined error.

Copy link
Copy Markdown

@rebeccahum rebeccahum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should add a check for jquery in the deps property to minimize blast radius.

Comment thread jsconcat.php Outdated
@rebeccahum
Copy link
Copy Markdown

Tagging @WPprodigy for a second set of eyes.

@WPprodigy
Copy link
Copy Markdown

I don't really love solving this just for when jquery specifically is a dependency, because in theory it should work for any type of dependency and only load an inline script after it's dependencies are met.

@BrookeDot
Copy link
Copy Markdown

Is it possible to get the $deps from the scripts and pass that to the array?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants