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 wp_add_inline_script function to ensure the inline scripts specify a type="text/template" attribute for inline scripts that are associated with deferred, when applicable #18

Closed
1 task done
10upsimon opened this issue Feb 9, 2023 · 7 comments · Fixed by #49
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

In order for applicable inline scripts to execute only once their deferred/async main/parent scripts have loaded (see issue 17), scripts should not execute automatically, and only on command. For this reason, type="text/template" should be added to said inline <script> tags. This prevents them from being executed by the browser.

onload handlers will be applied to the parent/main script in a future issue, ensuring that the inline module scripts be converted to JavaScript scripts, and then executed.

Acceptance Criteria

  • Enhance the existing WP_Scripts class, converting the printed inline script tags (where applicable) to be of type="text/template" as opposed to type="text/javascript" when they meet the business criteria as outlined in issue 17.

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 that meet the criteria as per issue 17 or of type="module".

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
@10upsimon 10upsimon changed the title Update the wp_add_inline_script function to ensure the inline scripts specify a type="module" attribute for inline scripts that are associated with deferred, when applicable Update the wp_add_inline_script function to ensure the inline scripts specify a type="text/template" attribute for inline scripts that are associated with deferred, when applicable Feb 9, 2023
@joemcgill
Copy link
Collaborator

I'm not sure if this has already been considered, but another option we could consider here is to load the inline scripts as a callback function on custom browser events that are fired as part of the onLoad handler. This way the inline script would be executed after the deferred/async script without the need for DOM manipulation.

@10upsimon
Copy link
Author

@joemcgill I'm not sure I understand the suggestion here. Can you provide a reference to or an example of such a custom browser event? If you're referring to a custom JavaScript event still, how can we avoid any form of DOM manipulation, even if we are to introduce a new script element as opposed to modifying an existing text/template one?

@joemcgill
Copy link
Collaborator

This may be a totally incoherent suggestion, but instead of printing the inline script into a script with type="text/template" that needs to be modified during onload handlers, I was thinking we could create a synthetic event and dispatch that event with the handle of the script during via the onLoad attribute of the main script. Here's a pseudocode example:

const wpScriptLoaded = new CustomEvent("wp-script-loaded", { detail: {handle: elem.dataset.handle });

// Main script.
<script src="{path}" defer data-handle="main-script" onLoad="() => document.dispatch(wpScriptLoaded)"

// Inline script, wrapped in a listener callback.
<script>
document.addEventListner(
  "wp-script-loaded",
  (e) => {
    if (e.detail.handle === 'main-script' {
      // Inline script printed here:
      ...do stuff.
    }
  }
);
</script>

Hopefully that explanation is more clear.

@10upsimon
Copy link
Author

Thanks @joemcgill and this is actually exactly what I was looking at last night, so we are indeed on the same page. I have no objection to this approach, do you @felixarntz ?

Browser support is probably good enough too: https://caniuse.com/?search=CustomEvent

@felixarntz
Copy link
Collaborator

@joemcgill @10upsimon We had discussed wrapping the inline script in logic at some point a few weeks/months back, but that is not feasible because it means the code is wrapped. We need to ensure the scope of the inline JS execution remains the same (e.g. global JS variables remain global).

That's why we should go with the text/template approach - it keeps the script content as is.

@joemcgill
Copy link
Collaborator

That reasoning makes sense to me. Thanks for confirming @felixarntz.

@10upsimon
Copy link
Author

Thanks @felixarntz , leaving this as is then and ready to proceed @kt-12

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