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

[FR] - Pass a promise to new props function #1126

Closed
nirazul opened this issue Jan 29, 2017 · 13 comments
Closed

[FR] - Pass a promise to new props function #1126

nirazul opened this issue Jan 29, 2017 · 13 comments

Comments

@nirazul
Copy link

nirazul commented Jan 29, 2017

I really like the new possibility to pass props to route components, thanks for implementing it!

As this would be a nice opportunity to use a props method as an API data fetcher, I'm intersted in passing a Promise either directly or returning one by this function.

I imagine something like this:

function asnycProps(route) {
  return new Promise((resolve, reject) => {
    ApiFramework.fetch(/* some route config */)
      .then((data) => {
        resolve(data);
      });
  });
}

const router = new VueRouter({
  mode: 'history',
  base: __dirname,
  routes: [
    { path: '/dynamic', component: Hello, props: asyncProps }
  ]
})
@posva
Copy link
Member

posva commented Jan 31, 2017

We can already do this with beforeRouteEnter:

beforeRouteEnter (to, from, next) {
    getPost(to.params.id, (err, post) => {
      if (err) {
        // display some global error message
        next(false)
      } else {
        next(vm => {
          vm.post = post
        })
      }
    })
  },

Props are meant to pass data that is relative to the route to its component in a more native way so you don't have to use $route.params or $route.path to check data.

@ghost
Copy link

ghost commented Jan 31, 2017

@posva yes, great, but props can be set post factum, after the vm initialized.
would be great to initialize vm with given props, just like in render function

@nirazul
Copy link
Author

nirazul commented Jan 31, 2017

@posva This would delay route rendering until the call returns data, which is exactly why I'd like to use props. An app feels much more responsive when the route changes and the data comes after, even if the route is an empty shell for the moment until the data is loaded.

What I've realized is, that in my understanding, the props should also be reinitialized when a dynamic route changes its params or query. So still, it's maybe not the best idea to use this approach...

@posva
Copy link
Member

posva commented Jan 31, 2017

If your view relies on some specific data being ready, you can move that part to a component and display when the view is ready. That way you clearly separate the logic of fetching and displaying

@nirazul You don't need any hook to achieve that: https://router.vuejs.org/en/advanced/data-fetching.html

@ghost
Copy link

ghost commented Jan 31, 2017

@posva sorry for insistent tries to be heard...
what about projects with hundreds components that all fetches data from same URL pattern?
Should we put same boilerplate code in every component?
Imo a way to automate this would be a nice to great feature.
Thanks.

@posva
Copy link
Member

posva commented Jan 31, 2017

No worries 🙂 . That data should not be fetched by the components but instead fetched at a view level or using something like vuex

@ghost
Copy link

ghost commented Jan 31, 2017

yes, already doing this, as described in #1123
the only issue is keeping fetched data in store.
what i'm asking about is a way to pass data from navigation guards directly into views, before their initialization.
Just tell me it's a anti-pattern and i'll shut up :)

@nirazul
Copy link
Author

nirazul commented Jan 31, 2017

@posva
You might want to update the example with the new beforeRouteUpdate hook as this is the recommended way to do, if I understood correctly:

// old
{
     watch: {
        // call again the method if the route changes
        '$route': 'fetchData'
    }
}

/* can be replaced with */

// new
{
    beforeRouteUpdate (to, from, next) {
        this.fetchData();
    }
}

@sleewoo
I have almost the same approach on data fetching as you've shown in your ticket. With the new hooks, it's easy to implement a route mixin and automate the fetching procedure:

export default {
    data() {
        return {
            // nobody likes undefined properties
            routeData: {}
        };
    },
    methods: {
        receiveFetched(fetched) {
            this.routeData = this.$set(this, 'routeData', fetched);
        }
    },
    beforeRouteEnter(to, from, next) {
        const Fetched = to.meta.fetch();
        next(vm => Fetched.then(vm.receiveFetched));
    },
    beforeRouteUpdate(to, from, next) {
        const Fetched = to.meta.fetch();
        Fetched.then(this.receiveFetched);
        next();
    }
};

Using this in a route is as easy as... :

import routeMixin from 'path/to/routeMixin';

export default {
    mixins: [routeMixin]
}

When switching routes, the fetch will be executed automatically and stored within the routeData object. Even when reusing routes after navigation, the beforeRouteUpdate hook will be triggered to update the data when needed.

You can combine this with a vuex cache store that keeps the responses via the fetch url and returns data from storage if available.

@ghost
Copy link

ghost commented Jan 31, 2017

@nirazul good approach, but after #973 this can be refined to a native solution.

Compare to this:

props: {
  env: { type: Object, required: true }
}

beforeRouteEnter(to, from, next) {
  to.meta.fetch().then((env) => next( { props: { env } } ))
}

Pros:

  1. view initialized with given props, just like with render functions
  2. no render until data is fetched, perfect for loading indicators
  3. no need to manually assign data

Cons:

  1. Anti-pattern cause data should be fetched in views???

Your method is even more attractive to me, but returning promises is sort of hairy, is not it? :)
Instead, props key can be left for static props and use asnycProps for data fetching.
It can receive the resolve as argument, just like functions for async components.

routes: [

  props: [
    'static',
    'dynamic'
  ],

  { 
    path: '/dynamic', 
    component: Hello, 
    props: { static: 'foo' },
    asnycProps(route, resolve) {
      route.meta.fetch().then((dynamic) => resolve( { props: { dynamic } } ))
    }
  }
]

and we get the view initialized with both static and dynamic props.

though not sure how to proceed when route's params/query changes :)

@nirazul
Copy link
Author

nirazul commented Feb 1, 2017

@sleewoo
That'd be a nice approach. Unfortunately, as you said, this doesn't solve the problem when the route is reused and only the params change. Also, I find it more pleasing for the route to change immediately and load the data afterwards. The app behaves more responsive that way.

I'll close this ticket, as all intentions are clear now and there's no intention to implement. Also, there's no clear path what to do on params/query change.

@nirazul nirazul closed this as completed Feb 1, 2017
@posva
Copy link
Member

posva commented Feb 1, 2017

@nirazul The documentation has to be updated indeed
@sleewoo you should do preventive display like {{ obj && obj.property }}. If you have a loader spinner you don't even need that because you can use a v-if/v-else:

<Spinner v-if="!obj"/>
<div v-else>
{{ obj.property }}
</div>

@rhyek
Copy link

rhyek commented Mar 2, 2017

@posva, @nirazul although your suggestions provide a solution, I think there is another way.

I like #973 very much because it leverages the use of props very nicely when trying to solve the problem of passing data from a route to a component in the most correct way. In my own issue, #992, I propose the option of passing props to a not-yet-created component during beforeRouteEnter. Currently, this hook only allows you to set data properties after the component has been created (next(vm => vm.someProp = resolvedData))

props are a way of providing bits of data to a component from an external source traditionally from a parent component or, less frequently, with propsData. Now, thanks to #973, it is also possible from a route definition. In all these cases, the data is available before a component is initialized.

Data coming over the wire is also an external source and, in my opinion and for consistency's sake, should be handled in a similar fashion. The most obvious way to do it would be to allow the props function in the route definition to optionally return a promise which, when resolved with the incoming data, allows us to continue with component creation passing along the data as props.

@posva:

{{ obj && obj.property }}

This is verbose and is a pattern I would like to not have to include in all my routable components. When you're designing a component that is meant to be called from a parent component, you don't have to do that because if you configure your props as required, you know that data is there at render time. Same should apply for API data.

<Spinner v-if="!obj"/>
<div v-else>
{{ obj.property }}
</div>

This is essentially what the data fetching section Fetching after navigation suggests and that's fine. This is how @nirazul prefers it, too.

IMO:

  • It is boilerplate I would not like to see repeated over and over in my application
  • It doesn't work with props so I can't have validation
  • It doesn't allow you to have things such as default handlers for API errors.
  • No separation of concerns. My component should always just assume the data is there and all handling of errors or loading animations should be done in a more appropriate and probably centralized place.

I would like to be able to do something like this:

// router.js
const router = new VueRouter({
  routes: [
    {
      path: '/people/:id',
      component: require('components/Person'),
      props(to) {
        return axios
          .get(`/api/people/${ to.params.id }`)
          .then(response => response.data)
      }
    }
  ],
  errorHandlers: {
    301: (to, from, next, error) {
      next(error.response.headers.Location)
    },
    400: require('components/BadRequest'),
    401: () => {
      store.commit('logout') // store will navigate to home route or whatever
    },
    404: require('components/NotFound')
    // 400 and 404 will cause Vue to render their respective components
    // while optionally having already navigated to the new URL
  }
})

router.beforeEach(() =>  {
  store.commit('loading', true) // shows loading spinner
})

router.afterEach(() => {
  store.commit('loading', false) // hides loading spinner
})
<!-- Person.vue -->
<template>
  <div>
    {{ person.name }} <!-- no error -->
  </div>
</template>

<script>
export default {
  props: {
    person: {
      type: Object,
      required: true
    }
  }
}
</script>
<!-- BadRequest.vue -->
<template>
  <div>
    <h1>Oops!</h1>
    <p>{{ error.response.data.message }}</p>
  </div>
</template>

<script>
export default {
  props: {
    error: { // from promise catch()
      type: Object,
      required: true
    }
  }
}
</script>

Another concern I've seen here is what happens when a route param changes. IMO, you just start over from the moment you call props and either handle any errors or pass the resolved data to the component. Only difference is this time around the component is already initialized. Same thing that happens when a child component's props are modified in the parent component.

@rhyek
Copy link

rhyek commented Mar 2, 2017

I realize this provides similar functionality to beforeRouteEnter and beforeRouteUpdate. Main differences are:

  1. Using props vs data (data available before or after component initialization)
  2. No need for two hooks, just one props function
  3. Possibility of having global HTTP error handlers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants