Introduce a simple prefetching solution #957

arunoda · 2017-02-02T11:56:28Z

Basically this is a universal solution which works everywhere.
We simply invoke a XHR for all the prefetching URLs.

But we don't parse JSON until the it's being used.

Fixes #956

arunoda · 2017-02-02T11:59:05Z

lib/prefetch.js


+  PREFETCHED_URLS[url] = fetch(url)


We could try to do this after the load event.
When we reach this page, we already passed DOMContentLoaded.

Since we don't do any processing, this should fine.

sarukuku · 2017-02-02T12:02:17Z

This is related to #956. 🤖

timneutkens · 2017-02-02T12:12:55Z

@sarukuku added to the introduction text so it's linked 👍

arunoda · 2017-02-02T13:08:03Z

@rauchg I did a load test. Basically, prefetch a lot of pages in a setInterval.

This is the Chrome's timeline profiler view.

Basically, it spend a pretty small(micro-secs) time to send the request and get it.
It's actually, smaller than the setInterval call.
(So, I think with the performance wise this is fine)

nkzawa · 2017-02-02T14:26:48Z

lib/router/router.js

-      const { Component, err, xhr } = routeInfo.data = await this.fetchComponent(route)
-      const ctx = { err, xhr, pathname, query }
+      const { Component, err } = routeInfo.data = await this.fetchComponent(route)
+      const ctx = { err, pathname, query }


Why we changed ctx ? I remember we use xhr for on error page.

The reason is the prefetched version doesn't have a xhr object. It'll see more about this.

nkzawa · 2017-02-02T14:30:48Z

lib/prefetch.js

-  return (typeof navigator !== 'undefined' && navigator.serviceWorker)
-}
+// Add fetch polyfill for older browsers
+import 'whatwg-fetch'


I wonder importing this module is ok since it would be loaded on server too ?

isomorphic-fetch maybe?

@nkzawa loading it's okay. But I think we need to guard it for the client side.
@mikker Yep. it would work, but we don't use this in the server side.

nkzawa · 2017-02-02T14:43:24Z

I wonder how this will affect the limit of concurrent HTTP requests on browser. (Is there the limit if we use service worker ?)

arunoda · 2017-02-02T16:29:03Z

I wonder how this will affect the limit of concurrent HTTP requests on browser. (Is there the limit if we use service worker ?)

@nkzawa I think I need to find more about that.

rauchg · 2017-02-02T18:27:43Z

By the way, if we're going to ditch the SW, I'm ok with prefetching by default and ditching next/prefetch.

arunoda · 2017-02-02T23:28:42Z

@rauchg I am not sure about that.
We can move next/prefetch logic into somewhere elas.
But I assume, user still has to tell us what to prefetch or not.

FrancosLab · 2017-02-03T01:28:56Z

Does ditching the service worker cost us any gains/losses vs using the service worker?

rauchg · 2017-02-03T01:29:30Z

We think this only has upsides so far

arunoda · 2017-02-03T09:35:52Z

@nkzawa actually it depends. Usually, it's around 4-8 connections per origin.

So, yes there's an issue with that. (But it applies for SW as well)
But I couldn't find any docs saying SW and limitations.

But with HTTP2 there won't be such problem because of multiplexing. But that'll affect the time to other resources in that origin.

So, here's my idea.

We can throttle to load just 2 JSON pages at once.
I'll move the codebase into the router.
I'll add prefetch APIs to the next/link.
Let's deprecate next/prefetch

msand · 2017-02-03T10:58:04Z

@nkzawa @arunoda

Even with HTTP2 multiplexing you have some device dependent limits on the number of concurrent requests, but it goes much higher. It can become limited by the memory allocated for the responses, somewhere around 25mb of pre-allocated header memory can easily cause problems even on desktop browsers. But with that you can easily afford 100+ concurrent requests, which should cover most needs. If you need more, it makes sense to use websockets and keep track of requests and responses in js.

These limits should apply both to service workers and other http requests. Also, when running out of your per-process memory, you will get e.g. net::ERR_INSUFFICIENT_RESOURCES in Chrome/ium.
https://chromium.googlesource.com/chromium/src/+/58.0.3001.2/content/browser/loader/resource_dispatcher_host_impl.cc#2341

Anyway, throttling prefetching at 2 concurrent requests is probably a sane default, considering mobile.

arunoda · 2017-02-06T17:31:05Z

@nkzawa Implemented what I suggested here.

I've changed the core router API to use fetch always.
That's cleaner.

I need to do few more tests and write some test cases.
In the meantime, could you check this?

arunoda · 2017-02-06T17:34:57Z

lib/prefetch.js

-    if (this.props.prefetch !== false) {
-      prefetch(href)
+    if (!linkPrinted) {
+      const message = '> You are using deprecated "next/prefetch". It will be removed with Next.js 2.0.\n' +


@nkzawa I suggested to drop this on 2.0 final release. I hope that's okay.

I'd like to @arunoda

nkzawa · 2017-02-06T17:44:57Z

I think xhr and fetch is not equivalent when you need to abort requests. xhr's abort should be more efficient ?

nkzawa · 2017-02-06T17:46:47Z

lib/router/router.js

@@ -185,7 +194,7 @@ export default class Router extends EventEmitter {

    try {
      const { Component, err, xhr } = routeInfo.data = await this.fetchComponent(route)
-      const ctx = { err, xhr, pathname, query }
+      const ctx = { err, pathname, query, xhr }


We should rename xhr if we replace it.

Yeah. That's on my todo list.
How about "jsonPageRes"

arunoda · 2017-02-07T00:12:38Z

@nkzawa Yep. With fetch we can't really abort. My suggestion is we don't need to abort.
With this, it's discard the response.

Usually, it's very rare xhr abort to happen. Even we do that, it might have reached the server.
So we don't get a big win.

Since we also do prefetching and has aggressive caching this won't be a problem.

arunoda · 2017-02-07T17:30:52Z

@nkzawa This is ready.

nkzawa · 2017-02-08T01:16:44Z

README.md

 #### With `<Link>`

-You can substitute your usage of `<Link>` with the default export of `next/prefetch`. For example:
+You can add `prefetch` prop to any `<Link>` and Next.js will prefetch those pages in the background.


I was thinking prefetching is the default behavior. What's the reason ?

Do we need to do that? I think it's better users turn it on.
So, we have no surprises.

👍 makes sense

nkzawa · 2017-02-08T01:19:18Z

lib/prefetch.js

-  let { pathname } = urlParse(href)
-  const url = `/_next/${__NEXT_DATA__.buildId}/pages${pathname}`
+let apiPrinted = false
+let linkPrinted = false


We'd like to use execOnce in ./util instead of having each flag.

I'll use it.

nkzawa · 2017-02-08T01:32:25Z

lib/router/router.js

+    // contain a map of response of prefetched routes
+    this.prefetchedRoutes = {}
+    this.prefetchingLockManager = new LockManager(2)
+    this.prefetchingRoutes = {}


I wonder if we can simplify this state managements by having just Promise of fetch ?
Additionally, I think state of prefetch and normal fetch results can be managed by an object.

Actually, we can't simplify. But we could move the isolate the fetching logic and move it to some other module. Since we are fetching in parallel, we need to keep these states.

Do you wanna move the fetching logic to somewhere else?

Maybe that's because of LockManager ? Then I will try to refactor after this was merged.

@nkzawa even with it or without it, since we are fetching parallel we've to do it.
In the previous version, we hadn't that check. So, it was possible to fetch the same page multiple times.

Let me give more about these states:

prefetchedRoutes - Store already prefetched routes

prefetchingRoutes - Keep track of currently prefetching routes. So, when we asked again to prefetch the same route(which it's prefetching), it won't happen.

prefetchingLockManager - Make sure, we only fetch two routes at once.

I think we shouldn't fetch the same page multiple times ?

@nkzawa we don't. But user could ask to do that. That's why we need to keep these states.

For an example, let's say user asking us to fetch following pages:

<Link prefetch href="/abc?aa=10" as="/abc/10" /> <Link prefetch href="/abc?aa=20" as="/abc/20" /> ...

But we only need to fetch /abc once. That's why.

nkzawa · 2017-02-08T01:58:21Z

lib/utils.js

@@ -53,3 +53,31 @@ export async function loadGetInitialProps (Component, ctx) {
  }
  return props
 }
+
+export class LockManager {


I think we can use https://github.com/timdp/es6-promise-pool or other existing modules. I feel the idea of lock doesn't fit to JS :D

I'd like to use an existing lib. But the promise pool and others similar doesn't work. They are good, if we know the amount of jobs we've do.

In our case, we don't know how many prefetches we need and when they arrive. So, what we need is a continuously running job manager with a max cap for parallel jobs.

Frankly, I couldn't find a something else.
Anyway, I like locks and would love to use them with JS.
Unlike other languages, memory accessing is thread safe :) So, I love it.

Anyway, if you know a lib which does what we want, let's use it.

es6-promise-pool looks you don't need to know the amount of jobs? And you can just recreate a pool if all existing jobs done.

@nkzawa But then we've to wait all the jobs getting done no? Usually we are getting requests by one by one.
Then we may need to do a setTimeout and check all the prefetches happening in the same event loop and so on.

But I thought this was nice than that.

Because, we no longer use it.

arunoda · 2017-02-15T08:52:19Z

Let's take this and keep iterate on this.

arunoda added 2 commits February 2, 2017 16:24

Implement a very simple prefetching solution.

84a939c

Remove next-prefetcher.

cbf8259

arunoda commented Feb 2, 2017

View reviewed changes

arunoda requested review from nkzawa and rauchg February 2, 2017 11:59

arunoda mentioned this pull request Feb 2, 2017

Failed to register a ServiceWorker: ServiceWorker script evaluation failed #956

Closed

nkzawa reviewed Feb 2, 2017

View reviewed changes

arunoda added 2 commits February 2, 2017 22:05

Require 'whatwg-fetch' only in the client.

fadf171

Use xhr in the code.

a379e12

arunoda added 2 commits February 6, 2017 14:47

Use a simple fetching solution.

adefd1b

Merge master.

1948db0

timneutkens mentioned this pull request Feb 6, 2017

Initial page load too slow #1013

Closed

arunoda added 4 commits February 6, 2017 19:14

Fix 404 and xhr status issue.

78f0cf5

Move the prefetching implementation to next/router.

91c189d

Add deprecated warnning for next/prefetch

32d564c

Run only 2 parellel prefetching request at a time.

f5c546c

arunoda commented Feb 6, 2017

View reviewed changes

nkzawa reviewed Feb 6, 2017

View reviewed changes

arunoda added 5 commits February 7, 2017 13:23

Change xhr to jsonPageRes.

a715de1

Improve the prefetching logic.

5e847ab

Add unit tests covering the Router.prefetch()

74ddfa7

Update examples to use the new syntax.

4f37ff7

Update docs.

12cec28

nkzawa reviewed Feb 8, 2017

View reviewed changes

arunoda added 3 commits February 8, 2017 08:44

Use execOnce() to manage warn printing.

b6a2cb1

Resolve conflicts.

66a834a

Remove prefetcher building from the flyfile.js

297cb81

Because, we no longer use it.

arunoda merged commit 14c86be into vercel:master Feb 15, 2017

arunoda deleted the new-prefetcher branch February 15, 2017 08:52

HaNdTriX mentioned this pull request Feb 26, 2017

Improve readme.md #1256

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Introduce a simple prefetching solution #957

Introduce a simple prefetching solution #957

Conversation

arunoda commented Feb 2, 2017 • edited by timneutkens Loading

Choose a reason for hiding this comment

sarukuku commented Feb 2, 2017

timneutkens commented Feb 2, 2017

arunoda commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkzawa commented Feb 2, 2017

arunoda commented Feb 2, 2017 • edited Loading

rauchg commented Feb 2, 2017

arunoda commented Feb 2, 2017

FrancosLab commented Feb 3, 2017

rauchg commented Feb 3, 2017

arunoda commented Feb 3, 2017 • edited Loading

msand commented Feb 3, 2017

arunoda commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkzawa commented Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda commented Feb 7, 2017

arunoda commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda commented Feb 15, 2017

arunoda commented Feb 2, 2017 •

edited by timneutkens

Loading

arunoda commented Feb 2, 2017 •

edited

Loading

arunoda commented Feb 3, 2017 •

edited

Loading

nkzawa commented Feb 6, 2017 •

edited

Loading

arunoda Feb 8, 2017 •

edited

Loading