Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WRasada
Copy link

@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

@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.

jsconcat.php Outdated Show resolved Hide resolved
@rebeccahum
Copy link

Tagging @WPprodigy for a second set of eyes.

@WPprodigy
Copy link

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

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