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

next/prefetch doesn't prefetch data #740

Closed
sedubois opened this issue Jan 11, 2017 · 36 comments
Closed

next/prefetch doesn't prefetch data #740

sedubois opened this issue Jan 11, 2017 · 36 comments

Comments

@sedubois
Copy link
Contributor

sedubois commented Jan 11, 2017

It is my understanding that replacing an import 'next/link' with import next/prefetch' should "just work", however I don't see this behaviour in my app. It doesn't use dynamic routes, and just has three links in its header which should be prefetched.

Locally, I see three pages in the cache with an "OK" response, which seems to be the desired behaviour:

screen shot 2017-01-11 at 15 02 05

When deployed with Now, I see the three pages in the cache, but there's no OK response (even when waiting several minutes):

screen shot 2017-01-11 at 15 01 41

But in both cases (local and deployed), I still see a "Loading..." indicator when I then click on the Discover link (and in general "feel slow", compared to when I return to the page a second time).

@impronunciable
Copy link
Contributor

impronunciable commented Jan 11, 2017

I'm not totally sure if this answer your question @sedubois but I implemented the log function on the prefetch service worker and this is what you see (look that about is fetched from cache)

2017-01-11 11 48 13

This is with next build && next start not the dev server

@sedubois
Copy link
Contributor Author

@impronunciable thanks but I don't have that behaviour in my case, the cache doesn't seem to be hit (didn't confirm with logs but is obvious from the behaviour). Looks like either I'm missing something or there's a bug.

@arunoda arunoda self-assigned this Jan 11, 2017
@arunoda
Copy link
Contributor

arunoda commented Jan 11, 2017

@sedubois let me check on this.
Could you send me a sample repo on this?

@rauchg
Copy link
Member

rauchg commented Jan 11, 2017

@sedubois are you using as ?

@arunoda
Copy link
Contributor

arunoda commented Jan 11, 2017

@sedubois I couldn't reproduce the issue. It's working pretty well for me.
Send me a sample repo.

Also check at the network tab too.
screen shot 2017-01-11 at 11 13 26 pm

If you see a hit with from ServiceWorkers as the size and there's no other related network hit. That's from the cache.

@sedubois
Copy link
Contributor Author

@arunoda the links to code and deployment on now are in the OP. Didn't use "as" because there are no dynamic routes (no custom server).

@timneutkens
Copy link
Member

@sedubois @arunoda just to be clear it's this app: https://github.com/RelateNow/relate

@adamsoffer
Copy link
Contributor

adamsoffer commented Jan 13, 2017

I can confirm this issue in my next apollo example as well. If you load the about page in my demo first and then navigate to the home page you'll notice a loading indicator which means that the whole page isn't getting prefetched. @sedubois perhaps it's related to using a HOC in our pages to fetch our data? Does prefetch not like that?

@arunoda
Copy link
Contributor

arunoda commented Jan 14, 2017

@ads1018 prefetch only does JSON page prefetching.(Basically the JS code) Not data.
So, in this case loading indicator is usual I guess.

I'll check that app too.

@adamsoffer
Copy link
Contributor

adamsoffer commented Jan 14, 2017

Ah ok good to know. This would be a good disclaimer to add to the docs. It would be a very useful/cool/powerful feature to enable optional prefetching of data as well.

@sedubois
Copy link
Contributor Author

sedubois commented Jan 14, 2017

Yes I think it would make sense to prefetch whatever kind of page there is. And if that's not possible for some reason the doc should make it clear.

I think this prefetch is a very important feature which would also open the way to offer some kind of offline mode and low-bandwidth friendly experience; it's also essential to the experience of progressive web apps I guess. So I'm very much looking forward to adding prefetching to my app :-) especially as im working on a mindfulness app, where people will for sure want to be able to use airplane mode to reduce their distractions (and also to download the audio/video contents in advance for a smoother and more mobile experience).

@arunoda
Copy link
Contributor

arunoda commented Jan 15, 2017

@ads1018 your app works fine.
It just doing prefetching well.
(As I said earlier, we only prefetch JS code related the page, not any data)

If data prefetching needed, you need to do it yourself.
We won't touch anything related to data.

@arunoda
Copy link
Contributor

arunoda commented Jan 15, 2017

@sedubois I tried your app (the deployed version). Prefetching works fine. (check the network tab)
But the caching tab in devtools doesn't show the response status.
Not sure, why is that?

BTW: I couldn't run your app. It needs a graphql server. Can I get a minimal repo I can test this case in more detail?

@sedubois
Copy link
Contributor Author

If data prefetching needed, you need to do it yourself.

@arunoda how is that data prefetching then done? Could we have an example? If as a user we need to do something special about data, the doc should be updated, because it makes it sound right now like there's nothing special to do about data: "Since Next.js server-renders your pages, this allows all the future interaction paths of your app to be instant. Effectively Next.js gives you the great initial download performance of a website, with the ahead-of-time download capabilities of an app."

But the caching tab in devtools doesn't show the response status.
Not sure, why is that?

I don't know. There's also no response status in @ads1018's app, and it also doesn't prefetch data in his app (you can see that by first loading the /about page, then navigating to the home). So you can use his app to repro the issue if easier (or I'll PM you the graphql backend URL if you need it).

@sedubois sedubois changed the title next/link doesn't prefetch page next/link doesn't prefetch data Jan 15, 2017
@arunoda
Copy link
Contributor

arunoda commented Jan 15, 2017

Data Prefetching

I think Next.js is about the app infrastructure, we always want user to handle the data. Relay, Apollo and others should take care of the data logic. We know nothing about the data in your app, so we can't prefetch.

You might say, we could just call getInitialProps() and cache it.

That's wrong. Data could change time to time. So, doing that is wrong since we don't know nothing about data.

What you can do is, rather using next/prefetch as a <Link/>, use our imperative API to prefetch JS code. And then use Apollo or other data layer to prefetch data.

I think having an example would be nice too.

@rauchg @nkzawa @impronunciable could you guys add something here.
May be as @sedubois suggested, we should add a note about prefetching section, that's this is not for data.

@adamsoffer
Copy link
Contributor

adamsoffer commented Jan 15, 2017

@arunoda I'm fetching data myself as you can see here. I just figured the service worker would reach this code. Perhaps, I need to check that the execution environment is a service worker?

Edit: As you explained above, I didn't realize the service worker doesn't call getInitialProps(). I guess that would explain it.

@arunoda
Copy link
Contributor

arunoda commented Jan 15, 2017

@ads1018 nope. Service Workers just don't read this code. It just download the source code.

@arunoda
Copy link
Contributor

arunoda commented Jan 15, 2017

I think we should write more about how Next.js works internally :)

@sedubois
Copy link
Contributor Author

@arunoda I still think prefetch should call getInitialProps. Even if the data changes meanwhile, it's a great "best guess" to instantly-render the page. And as soon as we navigate to the page, the data will be refreshed anyway because getInitialProps will be called again! Wouldn't that be the best of both worlds?

@sedubois sedubois changed the title next/link doesn't prefetch data next/prefetch doesn't prefetch data Jan 16, 2017
@sedubois
Copy link
Contributor Author

FYI, Gatsby 1.0 prefetches both code and data (ref). IMHO Next.js should too. As a user, I'd very much want that. I don't agree that Next.js "knows nothing about the data in your app", it actually explicitly created an API for it (getInitialProps). It can prefetch data, then refresh it when navigating to the page. (It could even in the longer term refresh its cache periodically, but that's getting more fancy.)

@rauchg
Copy link
Member

rauchg commented Jan 16, 2017

@sedubois If we decide to do it, it should be explicit. For example:

<Link prefetchProps={true} />

Otherwise it won't scale

@adamsoffer
Copy link
Contributor

Ooo @rauchg that would be nice. And agreed, developers run the risk of running up a user's data usage bill if the API is not explicit lol

@sedubois
Copy link
Contributor Author

sedubois commented Jan 16, 2017

@rauchg I think that'd be great 👍 🙂 ! And anyway if someone would want to prefetch all data, they could always alias Link to add prefetchProps automatically.

@rauchg
Copy link
Member

rauchg commented Jan 16, 2017

@ads1018 not just that bill, but the server's data computations can be very expensive, and only meant to be executed on-demand.

@gmaliandi
Copy link

Hey! Router.prefetch returns a promise with the prefetched page's source code, so you can eval that and call getInitialProps manually. It's a little bit hacky, but it might be useful - in my case for example, I make calls through a caching layer in getInitialProps, so just calling it is enough to populate the cache! :)

@sedubois
Copy link
Contributor Author

@gmaliandi interesting, do you foreseen any way to achieve this without using eval?

@gmaliandi
Copy link

@sedubois as far as I know, all you get in that response is the code of your page, i.e. the same code you are actually prefetching and storing for later execution - and next.js uses eval for that (which is perfectly fine). So if you just get that code and execute it (like next.js does), you're not exposing yourself to anything terrible.

If you definitely want to avoid using eval, you could technically parse only the subset of the code that you want. For example, you could add some sort of metadata in comments that indicate which stuff should be cached and then parse only that. But I think that would be less elegant.

In my opinion, this approach I commented makes sense mostly when you do caching in getInitialProps.

Eventually it would be cool to have some prefetchProps prop so this is done automatically by next (and if possible, even the whole page could be rendered in advance and stored in the cache, so the transition would be really fast).

@sedubois
Copy link
Contributor Author

@gmaliandi using eval doesn't seem so fine, see #1301

I agree that prefetching the page then pre-rendering its HTML would be a great feature to explore.

@adamsoffer
Copy link
Contributor

Indeed, it would be a great feature to explore. I created an issue for this specific feature - see #1175

@fabiodr
Copy link

fabiodr commented Apr 11, 2017

Maybe this code can help for Apollo case:

https://github.com/estrattonbailey/apollo-prefetch

@gmaliandi
Copy link

gmaliandi commented Dec 6, 2017

Hi! We just blogged about the approach I mentioned above. Hope this helps!

Fortunately since next.js no longer uses eval for fetching pages, it also no longer returns the source code as a string when prefetching programmatically, but rather returns the component constructor, so prefetching the initial props of a page no longer requires eval either!

@arunoda
Copy link
Contributor

arunoda commented Dec 7, 2017

@gmaliandi

That's a pretty impressive way of using Router.prefetch.

@gmaliandi
Copy link

Thanks @arunoda! We just published this to npm: data-prefetch-link.

@timneutkens
Copy link
Member

Going to close this issue. Core support is not needed thanks to @gmaliandi's brilliant work 👍

@Enalmada
Copy link
Contributor

It would be nice to bring this concept into core support now that it has been battle tested and matured for a while. I worry many new and existing Next.js users don't know that data-prefetch-link even exists. Having the "withData" option in the documentation would make it much more likely that the community would benefit from this brilliant concept.

@goranefbl
Copy link

Has anyone tested this with now? For some reason, its working just fine on localhost, but when pushed to now, its not prefetching.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants