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

Investigate Current Solutions for Deferred Scripts & Potential Interoperability Issues or Considerations #55

Closed
1 task done
10upsimon opened this issue Apr 5, 2023 · 8 comments
Assignees
Labels
needs:discussion This requires discussion to determine next steps. Script Loading Strategy Issues relating to the Script Loading Strategy (WPP)

Comments

@10upsimon
Copy link

10upsimon commented Apr 5, 2023

Description

There are currently a couple of know methods for implementing script strategy attributes on scripts printed via WordPress' native WP_Scripts api. There are also many plugins - some well known and others not - that offer script deferral methods as part of their optimisation suite.

This issue aims to outline the most popular approaches encountered, and potential considerations of each. This issue does not necessarily exist to solve for these issues, instead it exists to track potential issues and how we may choose to deal with the community and developer communications surrounding these, set up issues for future fixes etc.

The discovery process that was undertaken/is under way

Time was spent looking into how the community (mostly developers) and plugins/plugin authors handle the addition of defer and async attributes to scripts, and whether it may be considered worthwhile (potentially in a future enhancement) catering for various approaches that may result in conflicts with the new script loading strategy approach once this projects code is released into core.

Upon searching and browsing many forums, blogs, posts etc and looking into the various means currently at play for implementing deferred/async (and even lazy loaded) scripts, the most common 2x DIY approaches were of no real surprise:

Popular DIY Solutions

  • Via the script_loader_tag filter
  • Via the clean_url filter (Also suggested by WP Engine)
    • Essentially the latest hook in the output that one can use to safely bootstrap a script attribute to the output

Popular Approaches Taken by Plugins

Various plugins that defer scripts (including async) or execute other behaviours such as lazy loading of scripts all seem to operate on the principle of parsing the DOM using an output buffer method and running regex based search/replace of the HTML, manipulating the final HTML content stream and manipulating script tags to remove and/or inject the desired attributes. They mostly do this by starting the output buffer at the template_redirect hook. This is an approach taken by both lesser popular plugins such as Speed Booster Pack, as well as more popular plugins such as WP Rocket.

While some plugins are well implemented in the sense that they respect prior set attributes such as defer and async and skip over said scripts tags when these attributes are present, others (like Speed Booster Pack for example) do not, but do often make use of their own filter hooks that could be used to bypass the output buffer based DOM manipulation. This is a particularly important factor supporting the notion that community and developer communications surrounding the work we are doing, is of paramount importance.

Rocket Loader

When looking into WP Rocket specifically, we landed upon Rocker Loader, which appears to be an official CloudFlare script optimisation offering, no doubt supported by, implemented by or inspired by WP Rocket. Rocker Loader offers the ability to optimise script tags, including the ability to defer scripts. This can be bypassed via the addition of a specific script attribute: data-cfasync="false". See Ignore JavaScripts in Rocket Loader. This may be worthy of a mention in community comms, as it would directly break cores implementation of script loading strategies were it to process said WordPress sites HTML source code.

Considering the Community

In terms of whether it is decided that certain approaches need to be attempted to be undone by core, or whether it is decided that community and developer comms be the decided means for prevention, points to consider may be as follows:

  • For cases making use of the script_loader_tag filter, it could be a solved via a solution where core sets a super high priority hook callback and tries to undo attributes that are conflicting with what the eventual strategy is, and attempts to remove duplicate strategy keys, if present.
  • For cases that make use of the clean_url filter, there is no context of a script handle or script data passed, so this is likely a good example of something that can not/should not be fixed in core, and would require community awareness, i.e good comms. The fact, however, that large enterprise companies such as WP Engine recommend this approach means that it's probably more used that we'd like to think it is, and with no context of the script handle as part of the hook, it would be impossible to undo it.
  • For uses of the output buffering approach, this too is a challenging issue to solve for in core, we'd likely not even know that the context of the output buffer manipulation is, unless it offers a reasonably named action or filter against which it is run, but even then this sounds like a case for plugin developers to update their plugin logic in order to respect cores strategy, or provide a diversion tactic that is more "opt in" for the end user/developer.

Certain plugins such as Speed Booster Pack - although using an output buffer approach - do handle it via their own filter named sbp_output_buffer (to quote an example) , and they at least provide an easy enough exit path. Plugin developers should use these exit paths cleverly based on the introduction of the loading strategy work being done, and this can only be achieved by timely community comms around this work.

Usage of script_loader_tag Filter by Plugins

A quick glance at wpdirectory.net for the use of script_loader_tag (see results here) does produce a considerable amount of results, however it is believed that this is often used to apply strategies to the plugins own script(s).

Usage of clean_url Filter by Plugins

A quick glance at wpdirectory.net for the use of clean_url (see results here) also produces a sizeable amount of results, however it is clear that many of these plugins are utilizing it for means other than injected script loading attributes, while others such as Async Mos Speed up clearly are, but have such negligible market share.

Summary (Ongoing)

As it currently stands, the risk of severe backward incompatibilities feels low, but can not be ignored. Clear community and developer comms should be performed that empower both plugins authors and website developers to put into play the necessary guardrails that ensure that - where applicable - the strategies provided/implemented by core are respected.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@10upsimon 10upsimon self-assigned this Apr 5, 2023
@10upsimon 10upsimon added needs:discussion This requires discussion to determine next steps. Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) labels Apr 5, 2023
@10upsimon
Copy link
Author

@joemcgill @kt-12 I've created this as an initial issue against which I have tracked my findings surrounding potential interoperability issues related to how plugins and developers are solving script loading strategies "in the wild". Please feel free to comment, add suggestions and alter the contents as you see fit.

The TL;DR of it is that - while we can not ignore the fact that some incompatibilities may exist as a result of plugin and developer implementation - the greatest gift we can give the WordPress developer community surrounding this work, is the gift of good communication and great documentation so that plugin authors and web developers alike are empowered and informed to make the necessary decisions and implementations ahead of release into core.

@adamsilverstein
Copy link
Collaborator

This is an approach taken by both lesser popular plugins such as Speed Booster Pack, as well as more popular plugins such as WP Rocket.

Could these plugins switch to the core approach when available (at least as an option)? eg, enabling 'defer' by applying the 'defer' strategy to a set of (or all) plugin enqueues rather than directly into the html? Ultimately, I'd like to get plugins like WP rocket to implement custom strategies like 'worker' for 3p scripts.

The fact, however, that large enterprise companies such as WP Engine recommend this approach

Do you have a link to this reference? If would be good to get these updated with the new API.

For cases making use of the script_loader_tag filter, it could be a solved via a solution where core sets a super high priority hook callback and tries to undo attributes that are conflicting with what the eventual strategy is, and attempts to remove duplicate strategy keys, if present.

Rather than trying to work around these uses, I think we should promote developers adopting the new API. If they already do defer with script_loader_tag, they can use the new approach instead when it is available. The huge advantage for plugins is dependency handling.

@joemcgill
Copy link
Collaborator

This is really helpful research @10upsimon. My opinion is that we have no need to ensure that third party solutions are making use of the strategies registered by our enhancements to Core, but we should do our best to ensure that the addition of our code will not cause errors or regressions in third party code, if possible.

The best way to test this would probably be to create a test case or two that mimics the common ways these filters are applied and make sure that our code won't cause any regressions. For example, Yoast has an integration that adds an async attribute here: https://github.com/Yoast/wordpress-seo/blob/20.4/src/integrations/third-party/wordproof.php#L248, which would not be affected by our code, since it replaces the whole script string with their own, just swapping out the src attribute. However, other plugins are doing something like str_replace( '<script src', '<script async src', $tag );, which could result in multiple loading strategies being included in a single script tag.

I agree that in most cases, the filter strategies you have outlined are being used by plugins to add a strategy to one of their own scripts. In these cases, there should be no conflict since we're only enhancing the API, not applying strategies to existing scripts. If any plugins choose to use our new API, I assume they would also remove their filter approaches, so the plugins we need to be most careful about are the ones that are trying to perform optimizations across all scripts, like JetPack Boost. See: https://github.com/Automattic/jetpack/blob/5dcfe76bb621fecb87673f81b8324a68c9740628/projects/packages/assets/src/class-assets.php#L90

If we can't find and address any compatibility issues via code, then we need to make sure these types of issues are clearly highlighted in our developer notes.

Also, I don't think we should attempt to address misuses of the clean_url filter nor output buffering approaches, besides including good developer docs that encourage people to update their code to make use of the new functionality we're introducing instead.

@10upsimon
Copy link
Author

@joemcgill referencing some of your comments:

The best way to test this would probably be to create a test case or two that mimics the common ways these filters are applied and make sure that our code won't cause any regressions.

How best would this be done, via a clone of one of the existing tests that handles output of defer or async attributes, and applying one of the methods other plugins are using, such as the str_replace example? I am almost certain there will be duplicate attributes because we decided against checks to remove as such. I feel this could be addressed post merge, and also encourage developers to move to the new method, would you agree?

If any plugins choose to use our new API, I assume they would also remove their filter approaches, so the plugins we need to be most careful about are the ones that are trying to perform optimizations across all scripts, like JetPack Boost.

I agree, plugin authors would need to exercise due diligence and check for the WP version, applying the most applicable core or non core strategy. This we can address in our dev-note style documentation release.

Also, I don't think we should attempt to address misuses of the clean_url filter nor output buffering approaches, besides including good developer docs that encourage people to update their code to make use of the new functionality we're introducing instead.

I could not agree more.

@10upsimon
Copy link
Author

@adamsilverstein addressing your points:

Could these plugins switch to the core approach when available (at least as an option)? eg, enabling 'defer' by applying the 'defer' strategy to a set of (or all) plugin enqueues rather than directly into the html? Ultimately, I'd like to get plugins like WP rocket to implement custom strategies like 'worker' for 3p scripts.

Yes, I think our community docs should aim to encourage this as the go-to approach, I 100% expect vendors to replace custom with core implementations of loading strategies where able.

Do you have a link to this reference? If would be good to get these updated with the new API.

Yes, here is the WP Engine one for example, where they suggest clean_url as an approach to doing it custom: https://wpengine.com/resources/defer-parsing-javascript-wordpress/#Method_1_Deferring_Parsing_of_JavaScript_via_functionsphp

Rather than trying to work around these uses, I think we should promote developers adopting the new API. If they already do defer with script_loader_tag, they can use the new approach instead when it is available. The huge advantage for plugins is dependency handling.

We're all on the same page here, agree 100% with you on this.

@westonruter
Copy link
Collaborator

westonruter commented May 19, 2023

Usage of clean_url Filter by Plugins

A quick glance at wpdirectory.net for the use of clean_url (see results here) also produces a sizeable amount of results, however it is clear that many of these plugins are utilizing it for means other than injected script loading attributes, while others such as Async Mos Speed up clearly are, but have such negligible market share.

This is quite unfortunate, as it means in fact existing plugins using clean_url filter to inject the async attribute are relying on the fact that the script tags are currently printed with single quoted attribute values. This means migrating to using the script tag helper functions in #58 will break all such cases of this, since the wp_get_inline_script_tag() and wp_get_script_tag() functions use double-quoted attribute values. This means that in order to prevent breaking backwards compatibility, that PR will need to add an argument to opt for single-quoted strings.

This came up recently as well for the admin_body_class filter (Core-58336) where the lack of esc_attr() allows for additional attributes to be added to the body (as well as potential security vulnerabilities). I recall this was also a hack that used to be employed to add attributes to the body tag, by (ab)using the body_class filter (as well as adding attributes to div.post wrappers via post_class filter) which was stopped three years ago in Core-20009. See WordPress@d17a57a.

Nevertheless, given that hacks involving post_class, body_class, and (soon) admin_body_class were all put an end to in core, I wonder if the same should be considered for the clean_url filter. If plugins are abusing it to inject attributes, they need to be fixed.

Yes, here is the WP Engine one for example, where they suggest clean_url as an approach to doing it custom: https://wpengine.com/resources/defer-parsing-javascript-wordpress/#Method_1_Deferring_Parsing_of_JavaScript_via_functionsphp

WP Engine really needs to update that example to use the script_loader_tag filter instead (ideally with WP_HTML_Tag_Processor).

@westonruter
Copy link
Collaborator

I've proposed WordPress/WordPress-Coding-Standards#2241 to remove esc_url() and esc_url_raw() from the list of escaping functions from WordPress.Security.EscapeOutput. That could start nudging plugins away from using clean_url hacks to inject attributes, although clearly it would be just a part of the advocacy.

@joemcgill
Copy link
Collaborator

Closing this issue, as it's now basically complete and continued discussion is best moved elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:discussion This requires discussion to determine next steps. Script Loading Strategy Issues relating to the Script Loading Strategy (WPP)
Projects
No open projects
Development

No branches or pull requests

4 participants