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

Remove async attr on global JS script tag #2456

Merged
merged 1 commit into from Mar 23, 2022

Conversation

danielamorse
Copy link
Collaborator

@danielamorse danielamorse commented Mar 23, 2022

Jira

n/a

Summary

Fixes race condition caused by loading global JS file asynchronously.

Details

While reviewing #2423 I noticed sometimes the Floating Action Buttons JS would not load at all. This was because the JS was executing before the DOM was ready and the document query we were using to conditionally load the JS was returning empty.

At one point in time, when we were all-in on web components it might've made sense to load our JS asynchronously, as our JS would work whether it loaded before or after an HTML element was added to the DOM. But [async] doesn't make sense for anything not a web component that needs to query the DOM (most other components).

We also do not use [async] in production on any of our sites. So, I'm removing it from PL. This change only affects PL.

How to test

  • Review the code changes
  • Optionally, run locally and verify JS loads as expected

Copy link
Collaborator

@remydenton remydenton left a comment

Choose a reason for hiding this comment

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

Good reminder that we're not currently and also shouldn't async load in Drupal.

@danielamorse danielamorse merged commit cf6df72 into master Mar 23, 2022
@danielamorse danielamorse deleted the feature/pl-remove-async branch March 23, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants