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

Prevent Route Flickers #364

Closed
wants to merge 7 commits into from
Closed

Prevent Route Flickers #364

wants to merge 7 commits into from

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented Sep 9, 2017

This approach reworks the preact-cli-entrypoint & AsyncComponent so that the initial client-side mount won't wipe/reset the existing static/prerendered markup while waiting for chunks to finish loading.

Currently, the first bundle load kicks off the webpack instance (which initiates the Async loads). That sounds great (and it is!) unless you're on a bad connection and/or are in an emerging market. In those cases, what looked like a functional, ready page, will disappear (to your surprise) and you'll have to wait through another 5+ seconds until the app is back "online"

With this, the first bundle load kicks off the webpack instance (which still initiates the Async loads). The difference here is that it's now "rendering" to a cloned copy of document.body, but nothing here will affect the page. Its main purpose is to begin the async chunk-loading.

Once the chunk(s) have been fully loaded, a second render will take place, targeting the real document.body. In basic cases this will re-create the same markup, so when Preact enters the diff-phase, no changes are made. However, Preact now has full control of the client-side application, allowing libs like preact-router to take hold.

As @ajoslin pointed out, this approach does fire two sets of side effects (eg; a network request during componentWillMount x2), but anything attached to the fake/clone document.body will not yield any true visual effect. Also, this only applies to the very first load, since all other routes (eg, / ==> /about) are handled on the client-side only without any type of static-vs-jsx negotiation.

Sorry for the wall of text ~ 🙇

TL;DR:


Closes #281. Big thanks to @ajoslin for getting started on this.

- was always using the `require` built-in
- dispatch loading & loaded events
- track `ready` state value
- always use `child.default` key
- add `componentDidUpdate` for final event
- only render _if_ there is a child to render
- use initial render to kick-off async loads
- manage `inProgress` as escape-hatch var
- once “loaded” event fires, render to real tree
@ajoslin
Copy link

ajoslin commented Sep 9, 2017

I still don't know how I feel about mounting the app twice.. I can imagine some people thinking they can get away with some side effect that should really only ever run once, and then it runs twice and they get completely confused.

Nevertheless, I do think that the problem edge case (double-mount issue) is less common and less important than the case being fixed (always-flickering-in-every-prerendered-app). If there's a FAQ somewhere, maybe we should add a "My app's componentDidMount() is running twice! Why?"

And BTW, you mentioned this fixes a bug in emerging markets. That's true, but in reality this does effect every network speed. The flicker effect that this fixes shows up even on fast connections. On fast Wi-Fi, you still see 50-150ms of flicker in between "app shell load & render" (flicker) "home render" (end flicker).

All that to say it's an even more important fix in that it effects all users.

@lukeed
Copy link
Member Author

lukeed commented Sep 9, 2017

Yeah, I realize I was lucky enough to rarely see the flicker before your post. When I did see it, I thought it was a font-load issue or something.

The alternative to the "double" mounting is to create a built-in App HOC that dangerouslySetInnerHTMLs the existing root.innerHtml on first render only. The trick would be to somehow infer the dynamic portion (eg, <Router> w/ async chunks) from the static parts. We would then dangerouslySetInnerHTML + <Router />. This still initializes the chunk-fetch for the given route, without actually rendering anything additional to the page. The second render would pass a ready prop, which mounts the app itself & discards the previous innerHTML bit.

Ideally, there should be a Webpack-runtime function we can call pre-mount to know what needs to be loaded.... Maybe it exists? That would make this whole thing a breeze!

function init() {
  // basic init
}

function boot() {
  loadChunks().then(init);
}

@ajoslin
Copy link

ajoslin commented Sep 9, 2017

Your idea of the dangerouslySetInnerHTML sounds like the right route. It's similar to what I was thinking when I asked earlier if we could just tell Preact "when rendering, do not touch the inside of this element."

That might be the best solution... and maybe it's simple.

It might be as simple as having the app root element do the following...

function AppRoot ({ isLoading, children }) {
  const props = isLoading ? {} : {
    dangerouslySetInnerHTML: appRootElement.innerHTML
  }
  props.children = children
  return h('div', props)
}

props.isLoading is then set depending on the async component's loading DOMEvents.

The real question here is, if we set both dangerouslySetInnerHTML props and children props, what happens?

I'll give this a try tomorrow and see...

@ajoslin
Copy link

ajoslin commented Sep 10, 2017

Good news, looks like that approach might work (check the console): https://codesandbox.io/s/0o8jjvz7vv

You can dangerouslySetInnerHTML and set children. The children's componentDidMount event fires, but the end HTML is from dangerouslySet.

This means the app could be rendered, and the HTML set to the prerendered value. 😄

@lukeed
Copy link
Member Author

lukeed commented Sep 10, 2017

Hey @ajoslin, I had similar at one point.

Your one-off demo works, but it's not actually casting an AsyncComponent for which there is a loading period. The reason I mention that is because our <Home /> has to be rendered/called in order for the async-loading event to even fire.

Maybe I'm missing something, but here's how the flow would work with dangerouslySetInnerHTML -- notes taken from a local attempt I had going before this PR:

  1. Pre-rendered HTML markup

  2. With ready=false (default) client render uses dangerouslySetInnerHTML:

    // html ==> root.innerHTML, passed down
    h(props.root, props.ready ? {} : {
      dangerouslySetInnerHTML: { __html:props.html }
    })

    This is essentially:

    <App dangerouslySetInnerHTML={{ __html:props.html }} />
  3. <App/> mounts, discarding innards for hardset HTML

  4. <Home/> mounts, despite discard

    This also kicks off its lifecycles!

  5. AsyncComponent initiates via <Home />

  6. The route-home chunk begin loading asynchronously

  7. Chunk eventually loads, making ready=true

  8. Re-render from Root, but without dangerouslySetInnerHTML attribute.

  9. Repeat steps 3 & 4


Now, if you mounted to a throwaway div on Step 2, eg:

<div dangerouslySetInnerHTML={{ __html:props.html }} />

... you would never initiate AsyncComponent, which is the only way to actually trigger the chunk to begin loading.

@ajoslin
Copy link

ajoslin commented Sep 10, 2017

Alright, so I tried out the dangerouslySetInnerHTML method as we both described above. Unfortunately, it still results in a double componentDidMount. 😐

We should either go with your first solution or try a couple more things to try to get around the double mount.

Another solution is possibly to add a wrapper node around each route component with a deterministic selector. The prerender would then look like:

<div id="app">
  App Content
  <div class="route-wrapper" path="/">
    <div>Home Route Content</div>
  </div>
</div>

Then AsyncComponent could receive the path which it's prerendering as a prop (this is possible, I believe), and based upon the innerHTML of .route-wrapper[path="{path}"], render the prerendered content while it's loading:

AsyncComponent.prototype.componentWillMount = () => {
  const prerenderedEl = document.querySelector(`.route-wrapper[path="${this.props.path}"]`)
  if (prerenderedEl) {
    this.prerenderedHtml = prerenderedEl.innerHTML
  }
}
AsyncComponent.prototype.render = (state, props) => {
  // Now AsyncComponent will render a Vdom version of the prerendered route wrapper el...
  const parentProps = {
    path: props.path, 
    class: 'route-wrapper'
  }
  if (state.child) {
    parentProps.children = h(state.child, props)
  } else {
    parentProps.dangerouslySetInnerHTML = {__html: this.prerenderedHtml}
  }
  return h('div', parentProps)
}

Finally, during prerender we need to render these .route-wrapper[path="{path}"] elements around each route.

Would it be possible to do this with Webpack, by patching all requires in src/routes to be wrapped in an HOC?

This HOC would render .route-wrapper[path="{path}"] around each route component, but only when typeof window === 'undefined'.

@lukeed
Copy link
Member Author

lukeed commented Sep 10, 2017

Maybe. I still believe the true answer lies within Webpack since it's the master-orchestrator of what loads, when it loads, and when it's done. Preact does it's job perfectly as it is, shouldn't be fighting it.

We need some type of Manifest Plugin that maps pathnames to route-* chunk names.

// ignore *.js.map files here
var mapping = {
  '/': ['route-home'],
  '/about': ['route-about']
}

This would be inferred(?) and generated at compile-time and would be for static pages only -- no dynamic URLs since we don't pre-render those anyway.

Then the entry.js picks up a slight modification:

const { h, render } = require('preact');

// ...

function getApp() {
  let x = require('preact-cli-entrypoint');
  return h(x && x.default || x);
}

function init() {
  let app = getApp();
  root = render(app, body, root);
}

const mapping = { ... };
let chunks = mapping[location.pathname] || [];

if (chunks.length > 0) {
  // will be handled by webpack @ runtime
  require.ensure(chunks, init);
} else {
  init();
}

module.hot && module.hot.accept('preact-cli-entrypoint', init);

@ajoslin
Copy link

ajoslin commented Sep 11, 2017

Yeah, the compile time way really does sound the most reliable. The question is how to do it. One way that might be possible with prerender:

  • During prerender, intercept src/route imports and wrap them with our own internal wrapper component. This wrapper component looks for a path prop on render. Then while running prerender, for all path props found by this wrapper component, add them to some global array (manifest) by first converting the paths to regexp with their corresponding chunk name:

    function PrerenderRouteComponentWrapper (Cmp) {
        return (props) => {
            if (props.path) {
                pathChunkManifest.push({
                    regex: pathToRegExp(props.path), 
                    chunkName: getChunkName(props)
                })
            }
            return h(Cmp, props)
        }
    }
  • Add these regexes to an array in the bundle with a corresponding chunk name: pathChunkManifest = [... {regex, chunkName}]

  • On startup, iterate through the array, filter by matched paths, load chunks, and init:

     require.ensure(
         pathChunkManifest
             .filter(item => item.regex.test(window.location.pathname))
             .map(item => item.chunkName),
         init
     )

The downside is this would only work if the routing library people use follows path-to-regexp conventions and uses path props. But we could document that as a rule (if you want to use automatic route chunking, ensure your components in src/routes are rendered with a path prop).

Do you think this approach would work?

@lukeed
Copy link
Member Author

lukeed commented Sep 11, 2017

On startup, iterate through the array, filter by matched paths, load chunks, and init:

Webpack already splits each chunk into an array of all chunk pieces, so we can skip that step entirely.

The above code snippet I wrote will work. I've been with family all day today, but will start back up with this now. It's just a matter of printing the route:[chunks] mapping to the entry // making it accessible to the entry.

Also, it would need to be an exact match for the routes. Partly to enforce static-routes only && partly because / will match all routes and /foo may be a separate route from /foo/bar. Don't need to reinvent a route-matching library just to get a first-boot to work properly.

@developit
Copy link
Member

Random thoughts:

  • The async! loader is re-aliased to an identity function during prerendering, but it could instead be used to build up a list of routes.
  • Perhaps we could use the upcoming Router.getMatchingRoutes() method on the client here? Have a wrapper around the Router invoke that method during client bootup, find the current route, replace the component with one that does innerHTML preservation (as you guys figured out already), waits for the route's load method to resolve (specific to async-component, but that seems reasonable here), then swaps to the original defined route?

@CedarDCX
Copy link

Any progress update on this? I'm also experiencing the same issue where the page flickers on a slow connection.

@developit
Copy link
Member

I have a possible solution I can PR.

@lukeed
Copy link
Member Author

lukeed commented Feb 8, 2018

@developit Did you have that sumthin' sumthin'?

@lukeed
Copy link
Member Author

lukeed commented Feb 8, 2018

I got it 🍾 I think I'll open a new PR instead of updating this one~

@ajoslin
Copy link

ajoslin commented Feb 8, 2018

@lukeed my $100 bounty is still open on this too! 🍾

lukeed added a commit that referenced this pull request Feb 8, 2018
@lukeed lukeed closed this in #489 Feb 19, 2018
lukeed added a commit that referenced this pull request Feb 19, 2018
* FIX~! prevent route flickers 🎉

closes #364, #281

* fix linter / copy-paste error
@lukeed lukeed deleted the route-flicker branch March 21, 2018 23:49
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.

All routes are chunked, causing hydration of prerendered pages to flicker
4 participants