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

Discuss alternative ways for opts.waitFor #114

Closed
jonathaningram opened this issue Jan 11, 2018 · 5 comments
Closed

Discuss alternative ways for opts.waitFor #114

jonathaningram opened this issue Jan 11, 2018 · 5 comments
Labels

Comments

@jonathaningram
Copy link

opts.waitFor is good because you can put something like this in package.json:

{
  "reactSnap": {
    "waitFor": 5000
  }
}

And hopefully your requests will have loaded after 5s! But when crawling, some pages don't even have a fetch that needs to be waited on.

What about some other options for this?

Option 1: allow it to be a hash of paths:

{
  "reactSnap": {
    "waitFor": {
      "/page-that-fetches-from-slow-api": 5000,
      "/page-that-fetches-from-fast-api": 1000,
      "*": 500
  }
}

I used * to represent everything else - not sure if there's a better way.

Option 2: instead of using package.json, react-snap would have to allow JS to be run. I don't know, could there be a react-snap.js file at the root of your project with something like:

const opts = {
  waitFor: ({ pageUrl }) => { // I just passed `pageUrl` from puppeteer_utils.js - not sure if you'd actually want to pass `page` instead or as well...
    switch (pageUrl) {
      case "/page-that-fetches-from-slow-api":
        return 5000;
      case "/page-that-fetches-from-fast-api":
        return 1000;
      default:
        return 500;
   }
  }
};

export default opts

What do you think about this kind of API change?

Alternatively, do you think that the "waiting" feature is actually a symptom of something else? I've been wondering how crawling is actually meant to work - instead of this, is there a way for the crawler to know that the page is "stable", e.g. not waiting on any fetches, no outstanding renders or state changes to be performed. Just thinking out load.

@stereobooster
Copy link
Owner

crawler to know that the page is "stable"

networkidle- consider navigation to be finished when the network activity stays "idle" for at least networkIdleTimeout ms. From https://github.com/GoogleChrome/puppeteer/blob/v0.12.0/docs/api.md

Do you have project for which react-snap fails?

@jonathaningram
Copy link
Author

@stereobooster in the project I'm working on, it seemed if I didn't set waitFor then when rendering the page, the Loading... component is returned in the HTML. However, I cannot replicate it anymore, so we can close this out.

The only thing that I can think of that may have caused it is because the fetch is defined like this (pretty standard really):

class C {
  async load() {
    this.setState(() => ({ isFetching: true, error: null }));

    try {
      const resp = await fetch('something');
      const body = await resp.json();
      this.setState(() => ({
        response: body
      }));
    } catch (e) {
      console.error(e);
      this.setState(() => ({ error: e.message }));
    } finally {
      this.setState(() => ({ isFetching: false }));
    }
  }
}

As you can see, isFetching is only turned off in finally. I wonder if it's possible that the networkIdleTimeout is triggered (and thus SSR starts) before the finally is executed. Actually, isn't it even possible that the timeout occurs before the following code in the main try body even runs?

this.setState(() => ({
  response: body
}));

In which case the Loading... component will be the last thing rendered.

Also, did not know about networkIdleTimeout, thanks.

@stereobooster
Copy link
Owner

I thought about alternatives to networkidle. There are some alternatives (I need more time to write out options). I believe waitFor approach is not sustainable, it is error prone.

As of why you get Loading... error, I can suggest that this could happen due to network error. You can try to reproduce it with network disabled. If this is the case it should be possible to write code in react-snap which will catch this error.

@stereobooster
Copy link
Owner

Async snapshots

I believe there are two good approaches here:

networkidle

This is what react-snap uses. This technique is proved to be powerful enough to work in many cases without additional configuration. This is built-in puppeteer functionality - consider navigation to be finished when there are no more than 0 network connections for at least 500 ms.

Theoretically, it is possible to implement it for react-snapshot.

renderComplete

This idea was taken from Rendertron. The first call to renderComplete will disable default mechanism e.g. networkidle and prerenderer will start to wait for the second signal. It will be up to the user to implement when to call this function. Related: react#1739, react-transmit

Source: geelen/react-snapshot#80 (comment)

@loganpowell
Copy link

Can we use the renderComplete event with react-snap?

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

No branches or pull requests

3 participants