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

feat(ssr): ssrPrefetch option #9017

Merged
merged 11 commits into from
Dec 20, 2018
Merged

feat(ssr): ssrPrefetch option #9017

merged 11 commits into from
Dec 20, 2018

Conversation

Akryum
Copy link
Member

@Akryum Akryum commented Nov 3, 2018

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

  • New lifecycle hook: ssrPrefetch
  • New context hook: context.rendered = context => {}

@Akryum Akryum requested a review from yyx990803 November 3, 2018 00:02
@Akryum
Copy link
Member Author

Akryum commented Nov 3, 2018

@yyx990803 I added ssrPrefetch option merging

@Akryum Akryum changed the title feat: ssrPrefetch option feat(ssr): ssrPrefetch option Nov 3, 2018
@dylanmcgowan
Copy link

What's the status of merging this PR? I'm in npm version hell because of it and and my project is screaming at me with errors. The bottleneck is getting caused by version mismatches of vue-apollo nuxt and nuxt/apollo-module. SSR is very important. Hoping to see this resolved so I can deliver my product.

@yyx990803
Copy link
Member

@dylanmcgowan this is not a bug fix so likely unrelated to whatever error you are seeing.

@kevinmarrec
Copy link

@dylanmcgowan As Evan said it's not a bug fix around Vue itself, but a feature (as declared in PR).

The thing is that this new feature will allow @Akryum to fix issues within vue-apollo (vuejs/apollo#402 vuejs/apollo#392)

@Akryum
Copy link
Member Author

Akryum commented Dec 1, 2018

Related docs changes: https://github.com/vuejs/vue-ssr-docs/pull/214/files

@dylanmcgowan
Copy link

Pardon the late reply. To reiterate what @kevinmarrec said and to clarify why I commented in the first place, there are open issues with vue-apollo who's fixes depend on merging this PR. Fixing those bugs will allow even more fixes to be made in the wild west @nuxtjs/apollo world.

if (!Array.isArray(handlers)) handlers = [handlers]
try {
const promises = []
for (let i = 0, j = handlers.length; i < j; i++) {
Copy link

@galvez galvez Dec 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Akryum pardon me the intrusion -- just thought i'd drop this here -- I see that you're using a pattern for for loops that I used to employ myself, but, unless you assign the length to an external variable, this idiom actually makes things slower :(

@trainiac
Copy link

trainiac commented Dec 12, 2018

The more environment specific your hooks get the more likely you are to forget fetch data and have to repeat yourself. Let me demonstrate with a profile route component:

<template>
  <div> {{ user.name }} </div>
</template>
<script>
export default {
   ssrPrefetch() { 
     return this.$store.dispatch('loadUser', this.$route.params.userId)
   },
   computed: {
     user() {
         return this.$store.getters.users(this.$route.params.userId)
     }
   }
}
</script>

I'll test loading my route from the server and everything works fine. However, it's easy to forget that you have to add a way to fetch data for the client render context. To be fair @Akryum does a good job of mentioning this in the docs example. Let's fix up the route:

<template>
  <div v-if='user'> {{ user.name }} </div>
</template>
<script>
export default {
   ssrPrefetch() { 
     return this.$store.dispatch('loadUser', this.$route.params.userId)
   },
   mounted () {
     if(!this.user) {
        this.$store.dispatch('loadUser', this.$route.params.userId)
     }
   },
   computed: {
     user() {
         return this.$store.getters.users(this.$route.params.userId)
     }
   },
}
</script>

Ok getting closer. Now later on someone ends up adding links to this page that link to other user profiles. When clicking on those links, neither the ssrPrefetch nor the mounted hooks will get called because the component is rendered on the client and the component is already mounted. So we have to add another hook:

<template>
  <div v-if='user'> {{ user.name }} </div>
</template>
<script>
export default {
   ssrPrefetch() { 
     return this.$store.dispatch('loadUser', this.$route.params.userId)
   },
   watch:{
     $route (to, from) {
       this.$store.dispatch('loadUser', to.params.userId)
     },
   },
   mounted () {
     if(!this.user) {
        this.$store.dispatch('loadUser', this.$route.params.userId)
     }
   },
   computed: {
     user() {
         return this.$store.getters.users(this.$route.params.userId)
     }
   },
}
</script>

As one can see adding one piece of async data gets pretty complicated.

What if instead we could do something like this:

<template>
  <div v-if='user'> {{ user.name }} </div>
</template>
<script>
export default {
   routeData(store, to, from) { 
     return this.$store.dispatch('loadUser', this.$route.params.userId)
   },
   computed: {
     user() {
         return this.$store.getters.users(this.$route.params.userId)
     }
   },
}
</script>

This is achievable (won't go into implementation here) and the main advantage being:

Whether the route rendering context is server, client, or client update it will always get called so the first time you develop this it just works in all contexts.

Bigger Idea
Perhaps it is better to think about the significance of your component's data vs having to know the environment or request life cycle in which the data fetching happens. In my experience data fetching can be broken down into three categories:

  1. Permission data: Required to demonstrate permissions. This has to happen before the route is resolved in all render contexts: server, client, client update.
  2. Critical data: At this point the user has permission to see the page and there is some data that either needs to be rendered server side or needs to be fetched before the route is resolved. This is the trickiest category because it's behavior is the most subjective. While server side rendering should wait for this data but on client it' really up to the developer.
  3. Non-critical data: This data doesn't need to be rendered server side of before any initial render has taken place. It's supplementary stuff like likes or comments.

What if you could just drop your data management into those buckets and you never had to worry about the request context (i.e. server, client, client update)

I should write a blog post.

@Akryum
Copy link
Member Author

Akryum commented Dec 12, 2018

@trainiac ssrPrefetch purpose is to allow this kind of usages which where impossible to achieve previously. It will mainly be used internally by both Nuxt and vue-apollo to simplify SSR data fetching and allow fetching in any component (not just route-level ones). You are more to welcomed to create a library that does what you describe on top of it. 😉

@trainiac
Copy link

@Akryum Yes, I see that. It seems especially helpful for a GraphQL UI. Nice work! Hopefully my little blurb might help give others context to what this is trying to solve and some of the decision making that goes into where it's best to make API requests in an SSR app.

Might make sense to add the example of using beforeRouteUpdate in the SSR docs to fully cover all the hooks that need to be added in order make sure your data is always requested when you want.

@trainiac
Copy link

Or rather, since this PR is focused on enabling data fetching capabilities for all components, using a watcher on $route would be better for the docs vs beforeRouteUpdate which is only available to route components.

@skyline0705
Copy link

Looks very good!!!! if have this, I can do more things like executing unsubscribe in SSR mode when using vue-rx, which can prevent for the memory leak~~

@yyx990803 yyx990803 changed the base branch from dev to 2.6 December 20, 2018 19:13
@yyx990803 yyx990803 merged commit d7a533d into 2.6 Dec 20, 2018
2.6 automation moved this from In progress to Done Dec 20, 2018
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
@yyx990803 yyx990803 deleted the feat-ssrPrefetch branch May 20, 2022 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
2.6
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants