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 blocking JavaScript from AudioVideo integration #2389

Closed
jamesdonoh opened this issue Jul 10, 2019 · 2 comments
Closed

Remove blocking JavaScript from AudioVideo integration #2389

jamesdonoh opened this issue Jul 10, 2019 · 2 comments
Assignees
Labels
articles-av-epic Current focus for the articles features stream bug Something isn't working high-priority ws-articles Tasks for the WS Articles Team

Comments

@jamesdonoh
Copy link
Contributor

jamesdonoh commented Jul 10, 2019

Describe the bug
The initial implementation for integrating audio and video in #2151 loads RequireJS via a blocking external script tag added to the head.

This is very bad for performance because as soon as the browser encounters this tag it will block all further parsing of the page till the RequireJS library has been loaded over the network and evaluated.

This issue should take the existing implementation as it stands, to make it load lazily. This shouldn't be a massive issue that refactors a lot of code, and doesn't need to be best technical implementation, that will come in a future issue. This is just to stop blocking JS.

There should be a future ticket to carry out a more in-depth investigation on the best technical solution to lazy load entire react components.

Testing Notes

  • Test that requirejs is loaded asynchronously

To Reproduce
Steps to reproduce the behavior:

Either examine page source or to observe performance effect:

  1. Go to any Simorgh page
  2. Open DevTools and look at Network tab
  3. Notice blocking request for RequireJS in waterfall

Expected behavior
All external scripts should be loaded asynchronously.

Screenshots
Screenshot 2019-07-10 at 08 03 36

Additional context
Good overview from Ilya Grigorik at Google

@jamesdonoh jamesdonoh added bug Something isn't working ws-articles Tasks for the WS Articles Team articles-av-epic Current focus for the articles features stream labels Jul 10, 2019
@12
Copy link
Contributor

12 commented Jul 10, 2019

@jamesdonoh is this going to be blocking us from releasing video?

@jamesdonoh
Copy link
Contributor Author

Closing per #2456 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
articles-av-epic Current focus for the articles features stream bug Something isn't working high-priority ws-articles Tasks for the WS Articles Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants