Fix issue when requirejs/domReady could delay rendering of content

[ihor-sviziev]

Description (*)

This PR adds requirejs/domReady to head section just after requirejs library, so requirejs will not load requirejs/domReady himself.
As result browser starting loading all magento scripts earlier as there is no delay for loading domReady file anymore.

More info (including benchmarks): #23313 (comment)

Fixed Issues (if relevant)

1. requirejs/domReady.js can severely delay rendering of content #22909 : requirejs/domReady.js can severely delay rendering of content
2. Trigger page load listeners when no longer loading #23313 (comment)

Manual testing scenarios (*)

1. Add a non blocking asset to the page with a severe delay e.g. <img src="https://httpstat.us/404?sleep=10000" /> (I added this to Magento_Theme::html/absolute_footer.phtml)
2. Visit the homepage
3. The loading status indicator in the browser should be visible for at least the time set in the sleep value of the asset, however private / JavaScript content should render before this finishes (I used the Default welcome msg! text in the header to verify this)

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @ihor-sviziev. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[ihor-sviziev]

Added label "not required" because it was discussed already in #23313 (comment)

[ihor-sviziev]

@magento give me test instance

[magento-engcom-team]

Hi @ihor-sviziev. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @ihor-sviziev, here is your new Magento instance.
Admin access: https://pr-25527.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[ihor-sviziev]

@magento give me test instance

[magento-engcom-team]

Hi @ihor-sviziev. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @ihor-sviziev, here is your new Magento instance.
Admin access: https://pr-25527.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[krzksz]

@magento give me 2.3-develop instance

[magento-engcom-team]

Hi @krzksz. Thank you for your request. I'm working on Magento 2.3-develop instance for you

[magento-engcom-team]

Hi @krzksz, here is your Magento instance.
Admin access: https://i-25527-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

[krzksz]

@magento give me test instance

[magento-engcom-team]

Hi @krzksz. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @krzksz, here is your new Magento instance.
Admin access: https://pr-25527.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[ihor-sviziev]

There was mistake, it should be script, so I fixed it

[engcom-Alfa]

✔️ QA Passed

Result with slow 3g slow mode enabled in Google Chrome dev console:

Original - first load: 49.3 sec, second load: 12 sec;
With PR changes - first load: 21 sec, second load: 8.9 sec;

[DrewML]

Thanks for the hard work on this! I'll be reviewing asap

[DrewML]

Really appreciate the work here, and for trying out an alternative solution! Good to have options :)

Having said that, I'm not quite comfortable with the current implementation in this PR.

By design, the most critical content in Magento is rendered on the server, and we can consider most JS stuff on storefront pages "secondary content," as it will always be ready after the rest of the content.

Adding a synchronous script tag to the head for this functionality penalizes stores, because it delays the time to first paint and first meaningful paint. We want to prioritize the critical content as much as possible early in the rendering.

I'd consider adding domReady.js into the head to mostly be a workaround for the fact that Magento hasn't had a good bundling story. Luckily, we have baler now, which already bundles domReady into the core bundle loaded on the page, so folks that switch over will start seeing the benefits you see in this PR.

Will see if I can find some quick time to get some comparisons of the traces between this and #23313, to help visualize.

[DrewML]

@magento give me test instance

[magento-engcom-team]

Hi @DrewML. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @DrewML, here is your new Magento instance.
Admin access: https://pr-25527.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[ihor-sviziev]

@DrewML,
As baler is currently in Early Alpha state - unfortunately it can’t be recommended to be used on production, specially during holiday season, but this solution could be.

In comparison to #23313 we fixed comparison for state to all known states, it will prevent any issues in case if some browser will introduce any “initializing”, or some browser specific state.

Another thing that we fixed - loading script that detects if page is already loaded at first priority, that’s really terrible solution to load it in general queue as it’s on critical path of page load

[DrewML]

As baler is currently in Early Alpha state - unfortunately it can’t be recommended to be used on production

FWIW, we're a few weeks away from removing the "alpha" state on that. At this point, Baler has had more production usage that almost any other new feature we've released before, because we did the early alpha.

specially during holiday season, but this solution could be.

I'm not super familiar with our release cycle, but my understanding is that, if we merged this today, it wouldn't be in a release very soon, right? I just want to avoid a situation where we merge something we know we'll remove in a few months, because our backwards-compat requirements make that stuff difficult. If we know it's going to be pulled out shortly, it would be preferable for someone to publish a temporary patch for folks that want to try it out.

In comparison to #23313 we fixed comparison for state to all known states, it will prevent any issues in case if some browser will introduce any “initializing”, or some browser specific state.

Luckily this isn't something we have to worry much about. This check is an extremely common pattern, and both the WHATWG and W3C have strong requirements to not break the web when adding new features. But it's also a fairly trivial change to update that logic in the other PR, if we did go with it.

Another thing that we fixed - loading script that detects if page is already loaded at first priority, that’s really terrible solution to load it in general queue as it’s on critical path of page load

Agreed. But, while we don't want it to be low priority, we definitely don't want it to be the top priority, because there is other code in the graph that should be executing sooner. Let me do some tracing and come back here with some numbers on various metrics, that way we have less guessing to do :)

[DrewML]

Also worth mentioning that changing the script for domReady to a link tag with rel="preload" would have the same benefit in this implementation, without delaying the FP/FCP. We'd also then not need to change the define() to define('domReady')

[ihor-sviziev]

Hi @DrewML,
Is there anything that I should adjust?
Status of my PR isn’t clear, it has progress accept, but it doesn’t seems like it’s correct

[slavvka]

Hey @DrewML do you have any updates on this PR?

[slavvka]

The PR is under architecture review now

[DrewML]

If we can see traces showing that this improves key performance metrics without regressing any, I'd be happy to move forward.

But right now, this PR does not align with plans for front-end performance unfortunately. A goal in 2020 for me is to have everyone using baler, which the cloud team is working on making a reality for stores behind the scenes.

Moving domReady out of the graph and into a synchronous script in the head has 3 problems:

1. Will cause perf regressions when bundling (shipping domReady.js twice)
2. Delays the time to first contentful paint (because blocking in the head)
3. Treats domReady.js with a higher priority than scripts that run before the DOMContentLoaded event

Like all things performance, I'm happy to be proven wrong if there are traces that can tell a different story :)

[ihor-sviziev]

Hi @DrewML,
Thank you, I got your point. Together with baker probably my fix will have negative effect.
Thank you for your answer!
I’m closing my PR

[m2-assistant]

Hi @ihor-sviziev, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.