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

Update the functionality of adding inline scripts if the script on which the inline script is dependent has a deferred loading strategy #17

Closed
1 task done
10upsimon opened this issue Feb 9, 2023 · 10 comments · Fixed by #47
Assignees
Labels
19 Estimate of 19 points Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) type:enhancement New feature or request.
Milestone

Comments

@10upsimon
Copy link

10upsimon commented Feb 9, 2023

Description

The wp_add_inline_script, wp_localize_script and wp_set_script_translations functions can all be used to add inline javascript for a specific registered script handle. The former poses some notable challenges regarding the use of deferred scripts, whereas the latter two (wp_localize_script and wp_set_script_translations) are assumed to remain safe implementations for setting data before the the deferred scripts are loaded, as they are merely a means for defining globally accessible objects for use in said scripts and utilize the same underlying logic as the wp_add_inline_script function. Therefore, inline scripts added using either wp_localize_script or wp_set_script_translation can be automatically opted in to be standalone, ignoring the need to adapt to any specific loading strategies.

When employing the use of wp_add_inline_script attached to a handle who itself is deferred, there are considerations that need to be taken into account:

  • When using the after position, there is no way of (natively) knowing when a deferred script has been loaded.
  • When using the before position, we need to ensure that the inline script is present prior to deferred script(s) being loaded.

The manner in which these scenarios are dealt with is likely to be different for defer vs async loading strategies, these are outlined in more detail below.

When using inline scripts in the before position, the problem can be solved in a simple manner only if the main script of this before inline script has no dependencies, and by outputting the inline script in the header or footer as a blocking <script> element, it will inherently be executed and parsed prior to any deferred scripts being run, this is the nature of deferred scripts.

If however the main script has has dependencies that are earmarked for deferred loading, the before inline script should not be kept as blocking or it will break the execution cycle. In cases like these, and if the script is not opted in as being standalone (see issue here), the dependencies that are marked for deferred loading will need to become blocking.

On the other hand, when scripts utilize the after position, outcomes are easier to predict and handling the inline script execution is far easier. After scripts would only ever need to be deferred if their main script is deferred, as this means all of its dependencies are deferred as well. This deferral can be handled via onload handlers on the main script, see this issue for more detail (TBA)). The same principle applies to async loaded scripts, where any after inline scripts attached to said parent async loaded script can apply the same onload approach. Implementing said onload handlers, however, is out of the scope of this specific issue, and mentioned for context only.

This specific issue should aim to enhance the existing logic of the WP_Scripts class to do necessary due diligence to establish how before and after positioned scripts should execute so that when the onload handlers are introduced, it is easier to identify which inline scripts they need to act on.

Acceptance Criteria

  • Enhance the existing WP_Scripts class, adding and/or amending methods that digest the business rules outlined in this issues description to identify and mark inline scripts as needing specific onload handlers, based on the main/parent scripts eligible loading strategy.
  • Store these rules against the registered inline script using properties stored against the inline script object.
  • Validate that the stored rules are true and correct based on the logic outline in the issue description above, by inspecting the properties of inline scripts in various before/after locations, attached to main/parent scripts that utilize various loading strategies.

Unit Tests

  • Unit tests for the various processes surrounding wp_add_inline_script method currently reside in WordPress core source, starting at this location.
  • Add/edit the necessary unit tests to ensure that inline scripts meet the acceptable business criteria based on the business logic outline in this issue description.

Designs

No response

Describe alternatives you've considered

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@10upsimon 10upsimon converted this from a draft issue Feb 9, 2023
@10upsimon 10upsimon added type:enhancement New feature or request. Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) 19 Estimate of 19 points labels Feb 9, 2023
@10upsimon 10upsimon added this to the Milestone 3 milestone Feb 9, 2023
@joemcgill
Copy link
Collaborator

the dependencies that are marked for deferred loading will need to become blocking.

This makes sense to me because the inline script essentially inherits the dependencies of the script to which it is being added to (i.e., the script referenced by the $handle param), so those dependencies will need to be loaded with the appropriate strategy to support this inline script that is being registered.

It's worth noting that there are concat considerations with wp_add_inline_script that will need to be accounted for, either here or in a separate issue. See: https://core.trac.wordpress.org/ticket/14853#comment:41 and the resulting discussion.

On the other hand, when scripts utilize the after position, outcomes are easier to predict and handling the inline script execution is far easier.

One use case that I would be concerned about is the one described in that same concatenation discussion above, where an inline script is added to a registered script that is a dependency of other scripts in the queue. In some cases, it may be important that the inline script set to after is loaded before any subsequent scripts that are dependent on the initially registered script. In those cases, when an inline script is added to a deferred script, the deferred script may need to become blocking to ensure the inline script is executed before dependents are loaded, I think.

@joemcgill joemcgill assigned 10upsimon and unassigned joemcgill Feb 13, 2023
@10upsimon
Copy link
Author

@joemcgill to address this:

...In some cases, it may be important that the inline script set to after is loaded before any subsequent scripts that are dependent on the initially registered script. In those cases, when an inline script is added to a deferred script, the deferred script may need to become blocking to ensure the inline script is executed before dependents are loaded...

I'm not too convinced that this is a case worth solve for, as it becomes a very complex nature of events to solve if we are talking about multiple deferred scripts with inline dependencies that are actually reliant on the execution of more than one of those main scripts.

In my opinion, if there is such a strict requirement that inline script A fires after main script X but before main script Y, then it should just as well be set to a before inline script of main script Y I'd imagine, but even then we can not guarantee that script X has actually fully loaded/executed before script Y loads, as is the nature of deferred scripts.

However, further expanding my thoughts here, even if an inline script was supposed to execute only after a specific deferred main script, deferred scripts - while they do maintain load order - do not necessarily complete execution in the order they appear, making this a tricky scenario regardless.

I'm thinking specifically that for this use case you provide, there is not even a means via the API to specify that an inline script must executed after a certain script BUT before another when we're talking about deferred main scripts.

All in all, it sounds like a slightly rocky road to venture down.

cc @felixarntz as this was an area of the design doc you had a lot of valuable input on and may invaluable in this comment thread.

@felixarntz
Copy link
Collaborator

@10upsimon @joemcgill

In my opinion, if there is such a strict requirement that inline script A fires after main script X but before main script Y, then it should just as well be set to a before inline script of main script Y I'd imagine, but even then we can not guarantee that script X has actually fully loaded/executed before script Y loads, as is the nature of deferred scripts.

That's a fair point. I'm unsure whether that is a requirement; there is certainly a chance that this caveat could result in breakage, but I also think we should assess how real that problem is. Relying on an inline script of another script handle is probably not a good idea; those inline scripts are typically used to directly do something with the associated main script rather than being intended to be referenced elsewhere. This may be different with before inline scripts, but those are always blocking anyway, so there is no risk of breakage there.

I'd say we should continue to work on the implementation as we had outlined it and rely on testing as well as assessing how relevant or not that concern is. Of course there must be some script out there that does rely on a previous script's after inline script. But even then, we also need to keep in mind that deferring scripts is opt-in: This means, if a script really depends on a previous script's inline script, that script should probably just not opt in to deferring; as long as it remains blocking, it will force all its dependents (plus their inline scripts) to be blocking as well.

@joemcgill
Copy link
Collaborator

I generally agree that we don't need to support the specific use case of ensuring an after inline script is executed before scripts dependent on the script to which the inline script was attached. Specifically, due to this point that @felixarntz makes:

we also need to keep in mind that deferring scripts is opt-in.

I mainly wanted to raise this as a backward compatibility concern that we should keep in mind, since there have been real use cases in core, like the jQuery.noConflict() shim referenced in the conversation I linked to above, where the shim is conditionally loaded as an inline script that needs to run after jQuery loads, but before any scripts that rely on jQuery.

Perhaps this would be a good use case to include in our documentation as an example of where you would not want to defer a script that was a dependency of many others?

@felixarntz
Copy link
Collaborator

@joemcgill

Perhaps this would be a good use case to include in our documentation as an example of where you would not want to defer a script that was a dependency of many others?

Potentially. I think we should test this first to get a better idea how it behaves before discouraging deferring. Wouldn't that basically discourage deferring anything that depends on jQuery? If so, that would be a significant chunk of use-cases that wouldn't be able to benefit from a better loading strategy, so I think we need to validate that concern first.

@joemcgill
Copy link
Collaborator

I agree that we should test. In the above case, my assumption is that only jQuery would not be able to be deferred. Anything that depends on jQuery certainly should be able to be deferred.

@felixarntz
Copy link
Collaborator

@joemcgill Ah right, my bad. Deferring jQuery is probably infeasible anyway, given the large number of hacky in-content inline scripts that assume it is available.

@10upsimon
Copy link
Author

@joemcgill @felixarntz are we good to move this to To Do in your opinion?

@joemcgill
Copy link
Collaborator

Yes, I think we can move forward here with the understanding that any concatenation concerns will be handled separately from this issue.

@10upsimon
Copy link
Author

Thanks @joemcgill . cc @kt-12 let's be sure to not account for anything concatenation related at this stage, we likely won't ever need to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
19 Estimate of 19 points Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) type:enhancement New feature or request.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants