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

Puppeteer Page Event - 'domcontentloaded', 'load', 'networkidle0', 'networkidle2' #205

Closed
tomgallagher opened this issue May 23, 2018 · 8 comments

Comments

@tomgallagher
Copy link

Hey,

I've had a look at the code you're using to extract the styles (run.js) and I have a question / comment.

My understanding is that CSS styling tends to be render-blocking, whether it is in the form of a style tag, a link to an external style sheet, or inline styling on an element.

There is of course additional styling done by Javascript as and when that loads, whether it be render-blocking or async / defer.

From a performance point of view, there are two things that could usefully be done by a minimal css project. Firstly, identify the minimal css required above the fold to make sure that the first paint is properly styled. Make sure this is added to a style tag in the head somehow. Secondly, make sure that the total volume of css is only that required by the page, i.e. remove redundant selectors and their respective css rules from the download package.

Now, it seems to me that both the above could be achieved by waiting only for the 'domcontentloaded' event in Puppeteer. This event is quicker and more reliable in the wild than the 'load' event and the two 'networkidle' events. By definition, you can then use response.text() on a external stylesheet response to get all the rules and page.$$eval to assess those rules.

Seems to me to be a quicker and more reliable way to get the above-the-fold and present/not present elements and then divide the styling accordingly. Any styling applied by Javascript can be ignored.

@stereobooster
Copy link
Collaborator

So your only concern is what event used to detect finish of load e.g 'domcontentloaded' vs 'load' vs 'networkidle0' vs 'networkidle2' etc. Or you also asking for additional features like ("Make sure this is added to a style tag in the head somehow."). I would suggest to separate different requests in different tickets.

About additional feature - there can be another software which can do this based on minimalcss, for example react-snap does it. It is out of scope of current project.

About which event to use. It depends on situation, there are situations which require 'networkidle0', but I guess this it can be configurable (not sure if config exposed or not)

@tomgallagher
Copy link
Author

Yes, sorry about the post, it was more of a general discussion than it should have been.

I guess the basic question is: when would you need to wait for any event other than 'domcontentloaded' in order to get the styles for any given page?

@peterbe
Copy link
Owner

peterbe commented May 23, 2018

We (@stereobooster and I) debated this a lot in the past and concluded that the best thing is to just wait for networkidle0)

The reason being is many sites have some .css links and some .js (or inline Javascript for that matter) that waits for window.onload (or some incantation of it) and does JavaScript things. For example, if it used jQuery:

$(function() {
  // As soon as the page has loaded, extract all <h2> and <h3> tags and
  // generate a cute little Table of Contents near the top of the page.
  buildAndInsertTableOfContents($('.document h2, .document .h3'), $('.document .toc'));
});

That's just DOM manipulations and doesn't depend on the network. In the example, if the Table of Contents depends on some like .document .toc ul li selector in a .css file, we're want to make sure we're waiting for that to download and for the DOM to morph.

Another common thing is that on window.onload some sites fetch more stuff. For example:

// React example

componentDidMount {
  // Now that the page has rendered, download the related 
  // news (which is less critical) and display in the side-bar.
  fetch('/api/related-news').then(r => r.json())
  .then(news => this.setState({relatedNews: news}))
};

In both of these examples, by using networkidle0 we make sure we get the FINAL CSSOM after the page has "fully loaded" and calmed down.

It's just safer that way. If we try to extract minimalcss earlier, what MIGHT happen is that (as the per last example above) the related news is displayed in the side-bar without nice styling.

We recommended that you replace the render blocking CSS with the minimal/critical CSS but still load the rest of all the CSS later.

@peterbe
Copy link
Owner

peterbe commented May 23, 2018

Suppose that the DOM changes a bunch between domcontentloaded and networkidle0 (basically means in a browser when the loading icon spinner stops to spin) then it's likely to be very little. For example, in the example of the related news in the side-bar, that extra CSS is likely to be a less than 100 bytes. That extra weight to the inline <style> block isn't expensive. Especially when the HTML document is gzipped. However, the "cost" of having to look at unstyled lazy-loaded DOM elements is higher.

@peterbe
Copy link
Owner

peterbe commented May 23, 2018

Note that there's another interesting thing. There is a time between domcontentloaded and networkidle0.
Imagine the HTML from the server looks something like this:

<div class="toc toc-pending">Table on contents will be inserted here</div>

...together with...

.toc-pending {
  opacity: 0.7;
  color: #efefef;
}

And the .js file does something like this:

$(function() {
  // As soon as the page has loaded, extract all <h2> and <h3> tags and
  // generate a cute little Table of Contents near the top of the page.
  buildAndInsertTableOfContents($('.document h2, .document .h3'), $('.document .toc'));
  $('.document .toc').removeClass('toc-pending');
});

I.e. the DOM might look/styled a certain way that only lasts a couple of hundred milliseconds whilst the user is waiting for the lazy-loaded .js to change the DOM.

We're deliberately missing that opportunity. I.e. minimalcss won't include the .toc-pending CSS selector that was only used temporarily whilst waiting for the whole page to stop loading. Notes in the code here

@tomgallagher
Copy link
Author

OK. The reason I asked these questions is that I have done a lot of testing of Puppeteer in the wild and the networkidle0 event causes a huge number of timeout errors. In fact, so does the networkidle2 event. If you're collecting the CSS text as you go along, using the response.text() approach, then that should be OK as you have the info you need when the error is raised.

@peterbe
Copy link
Owner

peterbe commented May 23, 2018

Totally a valid point.

One argument that was made previously is that if you're so performance-hungry that you even use a tool like this you probably don't have a bunch of weird requests that timeout. You're likely to have bigger problems.

But we can wait and see. If the above statement is wrong, we could consider an option where you can actually control the thing to waitfor. E.g. "I really know what I'm doing and I know that only junk, that don't affect the first-render CSS, is going to time out."

@stereobooster
Copy link
Collaborator

Should we close this as non-actionable ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants