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

V2 API — Async snapshots #30

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

V2 API — Async snapshots #30

wants to merge 12 commits into from

Conversation

geelen
Copy link
Owner

@geelen geelen commented May 30, 2017

Hey friends! I have some FANCY NEW IDEAS™ about this library but would love some REAL HUMAN FEEDBACK™ from smart, good-looking people such as yourself.

Some backstory: React-snapshot started life as basically a zero-configuration static site generator, where you have a file system full of 'pages' or 'posts' or something, that it then gets crawled and generates all the static pages for. What I wanted was a version of Gatsby or Phenomic or static-site-generator-webpack-plugin or static-react except it was a three-line change from default Create React App install:

package.json:

- "build": "react-scripts build"
+ "build": "react-scripts build && react-snapshot"

index.js:

- import ReactDOM from 'react-dom'
+ import { render } from 'react-snapshot'
- ReactDOM.render(
+ render(

It worked pretty well, and after I fixed a couple of silly bugs that had been sitting around for several months it works even better. If you want a bit more context read the project Readme

However, I pretty quickly wanted to try this out on a non-static site, to see if there was a way to have the crawler snapshot a DB-driven application. This makes it a lot more similar to something like next.js, but again, I wanted the minimal possible changes from a vanilla CRA.

This is what I've been able to build so far:

+ import { snapshot } from 'react-snapshot'

class Home extends React.Component {
  state = { quotes: null }

  componentWillMount() {
+   snapshot(() => (
      fetch('/api/quotes')
        .then(response => response.json())
+   ))
    .then(quotes => {
      this.setState({ quotes })
    })
  }

  render() {
    const { quotes } = this.state
    return (
      <div className="Quotes">
        {
          quotes && quotes.map((quote, i) => <Quote key={i} quote={quote}/>)
        }
      </div>
    )
  }
}

During the initial app rendering at build time (using JSDOM), if any snapshot methods are called, we start tracking it, cache the result, then serialise the result alongside the static HTML that we produce. When the app gets booted on the client, that same snapshot method short-circuits, immediately calling the .then and the state gets populated before the render method is called. This means there's no flash, the checksum matches, and everything is JUST GREAT™.

I have to do some tricks behind the scenes to make this work—I'm not using a Promise internally because they're always asynchronous i.e. the render method would run with an empty state, then on the next tick (potentially after a paint) the setState call would fix things up. I simply stub about the then and catch methods to call them immediately. As far as I can tell, this will work JUST FINE™.

Basically, any async part of your app that gets data and returns a promise can be wrapped in a snapshot call and it'll get cached. Since there's no way to know which snapshot call is which, I simply assume that it'll happen in precisely the same order during build in JSDOM as in the client. This assumption also seems ok for the moment, but again there might be a case where it breaks down. For now though, things JUST WORK™.

I also expanded the API a little bit because these kinds of data-fetching components are super common and Next.JS has it's trendy async getInitialProps extension (which is too heavy for my liking) so I made like a hipster and carved out an artisanal HOC for my new library:

const Home = ({ quotes }) => (
  <div className="Quotes">
    {
      quotes && quotes.map((quote, i) => <Quote key={i} quote={quote}/>)
    }
  </div>
)

export default Snapshot({
  quotes: () => fetch('/api/quotes').then(resp => resp.json())
}).rendering(Home)

That rendering bit means "render Home even if I have no data". The alternative is thenRender, which will wait until all the promises have been resolved.

There's also another neat thing I just added while I was writing this PR where, if there's a chance your data will change more often than you'll deploy new snapshots, you might want to run the async code again and re-render when you've got the data hot & fresh off the API. That's easy:

  componentWillMount() {
-   snapshot(() => (
+   snapshot.repeat(() => (
      fetch('/api/quotes')
        .then(response => response.json())
    ))
    .then(quotes => {
      this.setState({ quotes })
    })
  }
- export default Snapshot({
+ export default Snapshot.repeat({
    quotes: () => fetch('/api/quotes').then(resp => resp.json())
  }).rendering(Home)

Basically, I don't want to try to cover the whole gamut of server-rendering use-cases, I'm just trying to solve the common use cases in the smallest amount of code possible. I want people to build more stuff in React using CRA but then have this super easy transition to hosting pre-rendered, critical-CSS-inlined, cached-on-a-nearby-CDN, reddit-frontpage-proof site.

What'd I'd like from you, smart & good-looking human, is to let me know what you think of the idea & implementation. Is the magic-synchronous-Promise & assumption of deterministic order too magic? Are methods like rendering and repeat worse than simple config objects? Or, and maybe this is the most important one, can you see a use case where this approach would break down so horribly that I shouldn't ship the API in its current state.

Just FYI, I do have this running on a demo site in production: http://react-snapshot-demo.pithy.af/. During deployment, the current API is crawled and snapshots for each page are taken. You can click around the whole site without JS, but if you have it on, it works like a normal React app, making API requests as it needs to. The homepage is using Snapshot.repeat but all other pages should not make a single API request if you reload the page. The code's here btw.

If you want to try it out on your own site, it's published as react-snapshot@next. Hope it works! ❤️

@superhighfives
Copy link

First off, this is wild, and you are wild for building it.

I like the use of HOC, and I think your assumptions are sound. One area I think could be tweaked are the method names for rendering and thenRender. Once I understood them as a human readable thing (snapshot this data, rendering home / snapshot this data, then render home) it made sense, but there may be something that is quicker to pick up. Some options:

  • renderImmediately / renderWhenResolved
  • render / renderOnComplete
  • rendering / thenRendering

(Or any mix of the first two.)

One other thing—should repeat be the default? As in, check the API's on load, and update? Perhaps have the non-rehydrating call as export default Snapshot.static({, where you stick with the snapshotted data. If not, maybe repeat could be rehydrate? It's certainly a term people seem to know.

I just clocked (lol) that with static you could set the whole thing up on a cron, scraping your own API and serving static HTML, cutting down the server overhead x 1,000,000. ╭( ・ㅂ・)و

This is so great, @geelen. This is so great.

@benschwarz
Copy link

One other thing—should repeat be the default?

Agree, this seems like the best default behaviour. In what cases would you not want this? That'd allow for CDN caching, as well as something that would work pretty well with a local serviceworker too, right?

If not, maybe repeat could be rehydrate? It's certainly a term people seem to know.

Going on that idea that it could 'rehydrate' by default — The method could be hydrate?

componentWillMount() {
-   snapshot(() => (
+   snapshot.hydrate(() => (
      fetch('/api/quotes')
        .then(response => response.json())
    ))
    .then(quotes => {
      this.setState({ quotes })
    })
  }

@geelen
Copy link
Owner Author

geelen commented May 30, 2017

Hmm, I'm not quite sure I follow. "Rehydration" is where there's a big chunk of data inline in the HTML that gets used to build up the exact React VDOM tree to match what's already there:

<script>window.react_snapshot_state = {"0":{"success":[[{" ...

That happens regardless. The question is whether, on the client, with all the data already there, do you go back to the API and see if anything's changed? If something has changed, suddenly data just appears out of nowhere. That's why it's a repetition.

I've just updated the DB behind the current deployment: http://react-snapshot-demo.pithy.af/. If you load that up now you should see a Roger Ebert quote quickly replaced by a Kurt Vonnegut one. It's jarring, and would require adding some extra code for a component to deal with the fact that new data might arrive.

On another page: http://react-snapshot-demo.pithy.af/authors/roger-ebert-2, you just get a single quote, even though I've added another quote into the DB. If you navigate (click "Review of the film North" then hit back), the fresh data is retrieved, but refreshing the page you'll go back to just one. So it's not ideal either, but it's certainly going to come up less often.

In each case it's made worse by just how simple the demo app is. If you were using any kind of centralised data store like Redux you'd have very different ways of managing data & API access. And, if you dropped in something like react-flip-move you could make sudden appearances of data look a lot better.

But the real problem is letting snapshots and the DB get out of sync, so, naturally you need to consider how often your DB changes. You'd never use this for a twitter homepage but it would certainly work for a blog with updates every once in a while or a news site with multiple changes a day. You just need to regenerate snapshots when new data is there to be served (and if you do that, I don't think .repeat should be turned on by default).

@benschwarz
Copy link

That happens regardless. The question is whether, on the client, with all the data already there, do you go back to the API and see if anything's changed? If something has changed, suddenly data just appears out of nowhere. That's why it's a repetition.

Isn't that the 'rehydration' process? Something is loaded, then rehydrated with fresh content? At least that's how I'd understood it previously anyway.

If something has changed, suddenly data just appears out of nowhere.

Hmm, that brings up a UI question—is there a way to get a handle on that update cycle? So customers can be shown a 'Loading fresh content' message?

@geelen
Copy link
Owner Author

geelen commented May 31, 2017

Nope rehydrate doesn't hit the API at all, it's about booting the app with the data that's already present: https://stackoverflow.com/a/29826133

@benschwarz
Copy link

benschwarz commented May 31, 2017

I like the terminology that @superhighfives used around this then.

@superhighfives
Copy link

In that case, perhaps resync instead of repeat?

Also, wa-hey, I did not realise that was the definition of rehydrate, @geelen. That makes sense, for sure. I suppose our difference is that we're dealing with static pages, so the server doesn't have the opportunity to pull the latest state from the API for the initial render. That said, you're hitting the API the same amount of times—once—given it's a static page vs getting state from the server.

componentWillMount() {
- snapshot(() => (
+ snapshot.resync(() => (
    fetch('/api/quotes')
      .then(response => response.json())
  ))
  .then(quotes => {
    this.setState({ quotes })
  })
}

Alternatively have it as a setting option?

componentWillMount() {
- snapshot.resync(() => (
+ snapshot(() => (
    fetch('/api/quotes')
      .then(response => response.json())
- ))
+ ), {resync: true})
  .then(quotes => {
    this.setState({ quotes })
  })
}

That's a little less clear though, x 1,000,000.

I still lean toward automatically hitting the API / resync as default, as it solves a bunch of the issues you've mentioned around losing sync, especially if the API is always serving custom content (for example, a user logged into Twitter). But I also see where you're coming from—regenerating your snapshots should happen as often as your app requires, and that's a case by case thing. Ultimately the option is there, and that's what matters.

@superhighfives
Copy link

superhighfives commented May 31, 2017

Nope rehydrate doesn't hit the API at all, it's about booting the app with the data that's already present: https://stackoverflow.com/a/29826133

Doesn't the server hit the API? That's semantics, though.

@geelen
Copy link
Owner Author

geelen commented Jun 1, 2017

Nah when the server/build hits the API that's just normal execution. Then it takes the result & dehydrates it (serialises it into the HTML). When the app boots on the client, it sees the data in the HTML and rehydrates (parses). Basically it's like you've started the program running on one computer, paused it, then resumed it on another.

I think resync is a good name but yeah I don't think it should be the default. Also I think render and renderWhenResolved are better than what I've got currently. Will update.

@superhighfives
Copy link

This has been very educational. 👍

@alexeyraspopov
Copy link
Contributor

@geelen, async rendering works great so far! I only faced one issue related to webpack's code splitting but it can be related to the base version, not only this branch. Whenever I use import(), webpack creates a chunk which is going to be included to the doc dynamically. The chunk is added as a <script> in the <head> of the document. However, the main bundle is always present only at the end of the <body>. That means, chuck is going to be loaded earlier than main bundle.

Webpack uses async attribute for the script element, and ideally, in the static document, it means that the script is going to be executed much later (later after the main bundle loaded from the bottom of the body). What happens to me, is that react-snapshot captures loaded chunk in the <head> but async attribute is missing. In this case, document will try to execute the chunk before rendering the content. Execution fails because chunk cannot use webpackJsonp() function which is utility of webpack but it's not yet present since main bundle is not yet loaded.

While I was writing all this text, I've found the original issue: jsdom/jsdom#1564. I cannot confirm that latest version of jsdom fixes it, but it worth trying.

@alexeyraspopov
Copy link
Contributor

alexeyraspopov commented Jun 30, 2017

I've tried to think about additional cases related to async modules loading and it seems that the better approach would be to remove those scripts from <head> at all, so they will remain their dynamic nature (since webpack will attempt to load them one more time). I have a case where one lib is used under a single particular scenario and its size is bigger than all other bundles together. In this case I want it to be loaded dynamically in the exact time I need it.

@geelen
Copy link
Owner Author

geelen commented Jun 30, 2017

@alexeyraspopov very interesting. I've been thinking about making react-snapshot use chrome headless instead, because these kinds of bugs are irritating (and potentially endless)

But that's cool, glad it's (mostly) working!!

@alexeyraspopov
Copy link
Contributor

@geelen, I'm going to build a reference app using react-router, react-coroutine, import() and some external API. Will post it here with all the issues/caveats I faced.

@geelen
Copy link
Owner Author

geelen commented Jun 30, 2017

@alexeyraspopov OMG thank you that sounds amazing. I'm off on holidays the next 3 weeks so I won't be able to make core updates but I'll be around on emails to bounce ideas around ❤️

@thisRaptori
Copy link

Hey @geelen, really liking this so far! We're testing it out with API calls via redux, and have hit a bit of an issue when there are multiple requests.

From what I can tell, snapshot is recording duplicate requests - for example when there are three requests made, the window.react_snapshot_state variable in the file contains six objects:

window.react_snapshot_state = {
  {0: {response_one}},
  {1: {response_one}},
  {2: {response_two}},
  {3: {response_two}},
  {4: {response_three}},
  {5: {response_three}}
}

Inside the app it appears that snapshot expects the correct number of requests, and as a result it loads snapshot_state[1] when it's expecting {response_two}, which breaks everything.

Happy to put together a simple example if it'd help, but here's an idea of how we've structured our async actions:

const asyncFetch = () => dispatch => {
  dispatch(asyncRequest());

  const url = 'api-url-goes-here';
  const headers = new Headers();
  headers.append('Content-Type', 'application/json');

  snapshot(() => (
    fetch(url, { headers, method: 'get' })
    .then(response => response.json())
    .then(json => parseData(json))
    .catch(error => ({ error }))
  ))
  .then(response => {
    if (response.error) {
      dispatch(asyncError(response.error));
    } else {
      dispatch(asyncSuccess(response.payload));
    }
  });
};

Is there something wrong about what we're doing which might be causing the bug?

@geelen
Copy link
Owner Author

geelen commented Aug 1, 2017

Hmm, nothing looks wrong at first glance... Unless somehow you're calling asyncFetch more than once? I've just published a new prerelease with a slightly different strategy for rendering, could you give it a try?

@thisRaptori
Copy link

The updated version didn't fix it, so I looked through the logic again and you're right - one of the components was looking for a flag in the wrong place, and sent a duplicate fetch request in some situations.

Fixed that, and it all works perfectly now, so thanks for the help! 😄

@geelen
Copy link
Owner Author

geelen commented Aug 2, 2017

I guess that means the updated version didn't break anything new either, so 🎉

Looking at how things might change with React 16+. It's quite likely react-snapshot 2 will require 16 and up because of big changes to the SSR stuff. Would that be an issue for you?

@thisRaptori
Copy link

Yeah true - fairly sure it was a little faster too! 😁

I don't know actually, though from what I remember there aren't any breaking changes being introduced? Doubt there'll be any issues, since we're able to be pretty flexible on stuff like that; as long as it doesn't require a big refactor we'll probably test it out pretty soon. 🙂

@jasan-s
Copy link

jasan-s commented Aug 14, 2017

how canI use snapshot when rehydrating persistent redux sttore using redux-persist.

@abhisuri97
Copy link

abhisuri97 commented Aug 30, 2017

Hey @geelen this is really great. But I am having trouble getting this to work with my redux actions. Basically I have a route that loads md files with text dynamically based on the prop passed to it (e.g. domain.com/page/test will fetch the test.md file from my public static dir and display it in the component. I am trying to figure out if there is some way to render the page. Note I am hosting everything on surge as a frontend site.

Code for reference

In actions.js:

export function fetchPageContent(path) {
   return dispatch => {
      return snapshot(() => ( 
        fetch(`/content/${path}.md`)
          .then(res => dispatch(receiveContent(res))
        )
      )
   }
}

in Page.js

class Page extends Component {
  constructor(props) { super(props) }
  componentWillMount() { 
    const { dispatch } = this.props;
    // i store the path in this.props.location.state in React Router
    dispatch(fetchPageContent(this.props.location.state)) 
  }
  render() {
    return ( <div>{ this.props.content }</div> )
  }
}

function mapStateToProps(state) { return state }
export default withRouter(connect(mapStateToProps)(Page))

Currently absolutely nothing shows up in the window.react_snapshot_state (other than an empty object) for pages on that component. Is there something I am doing wrong here?

Another edit: When I move all the fetch logic into the component itself, everything works as expected; however, I do need all the logic to be in a redux store.

@mrtysn
Copy link

mrtysn commented Oct 16, 2017

Hello @geelen, I am very excited that I found this pull request. Are there any updates on this? If you need any, I am willing to help.

@stereobooster
Copy link

stereobooster commented Dec 27, 2017

@geelen there is a simpler approach. We can repeat what puppeteer does. Puppeteer waits 0.5 seconds after last network request made. All we need is to catch all network requests, store each as a promise into an array, wait for all, wait for 0.5 seconds more. That's all!

const requests = [];
window.fetch = (path, options) => {
  path = url.resolve(window.location.toString(), path);
  const response = fetch(path, options);
  requests.push(response);
  return response;
};
...
if (requests.length > 0) {
  Promise.all(requests).then(...)
}

Trying to do something similar stereobooster/react-snap#71

@stereobooster
Copy link

Rendertron approach

/**
 * Listens for changes to the 'renderComplete' flag.
 */
function listenToCompletionFlag() {
  Object.defineProperty(window, 'renderComplete', {
    set: function(value) {
      if (value == false) {
        console.log('Rendertron: Waiting for rendering flag');
      } else if (value == true) {
        console.log('Rendertron: Rendering complete');
      }
    }
  });
}

// Check if an explicit `renderComplete` flag is being used.
let waitForFlag = false;
Console.messageAdded((event) => {
  if (event.message.text === LOG_WAITING_FOR_FLAG) {
    waitForFlag = true;
  } else if (event.message.text === LOG_RENDER_COMPLETE) {
    pageReady();
  }
});

@lukerollans
Copy link

This is great, and precisely what we need for a project we'll be launching soon. Although, we are using Apollo for GraphQL connectivity. I'm quite new to Apollo, so unsure what its internals are like and how I could translate its HOC pattern to the snapshot promise.

If there's any examples available that I haven't been able to find, it would be super duper appreciated!

@zomars
Copy link

zomars commented Jan 7, 2019

Is this still active?

@superhighfives
Copy link

@zomars, I believe https://github.com/stereobooster/react-snap is a more actively maintained take on the core principles of react-snapshot. cc @geelen

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

Successfully merging this pull request may close these issues.