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

Simplify transition hooks #321

Closed
yyx990803 opened this issue Jan 11, 2016 · 15 comments
Closed

Simplify transition hooks #321

yyx990803 opened this issue Jan 11, 2016 · 15 comments

Comments

@yyx990803
Copy link
Member

Here's what I think of the current API:

  • The current list of hooks is a bit overwhelming. Do we actually need canActivate and canDeactivate? It seems activate and deactivate can already handle most use cases with transition.abort() and transition.redirect().
  • The data hook name is not that self-explanatory.
  • The way we determine the async-ness of these hooks is convoluted. Sync, Promises, callbacks... we should try to simplify it.

Idea

Some potential breaking changes for 0.8.0:

  • Remove canActivate and canDeactivate.
  • (Maybe?) Remove canReuse. The same component is always reusable. Handle route change in onChange.
  • Rename hooks, now we have just 3 of them:
    • onEnter: what activate used to be.
    • onLeave: what deactivate used to be.
    • onChange: what data used to be. The new name better explains when it is called (on every route change when the current component is active)
  • In addition: allow specifying onEnter and onLeave in the route config as well.
  • Open to suggestion: how do we simplify hook async-ness?
@yyx990803 yyx990803 added this to the 0.8.0 milestone Jan 11, 2016
@chenkie
Copy link

chenkie commented Jan 11, 2016

I think one of the nice things about hooks like canActivate is that intent is expressed nicely in the hook name. An example of this is checking authentication status. I think being explicit like this has some value for readability.

@amirrustam
Copy link

@chenkie as a general rule of thumb explicit naming is useful and a good practice, though I think in this context @yyx990803's proposal is a good start.

One of the things that attracted myself and others to Vue was how its simple API is small enough for you to keep a mental model of it in your mind. A minimized set of transition hooks with more intuitive names will help maintain this trait of Vue. For example, I found the Vue instance lifecycle hooks to be very self-explanatory and along with the provided diagram I was able to have mental model of how everything worked.

When first reading through the docs, I found the name of the data hook odd, and I know an issue I responded to the other day was posted due to the confusion around this hook.

Now, having more hooks does provide more granular control within the transition pipeline, though I don't particularly see a loss of functionality with the proposed 3 hooks. So a +1 for the proposed hook names.

Have to do some thinking about the async stuff.

@benseitz
Copy link

👍 I like the simplification!

@utopiaio
Copy link

I like that the hooks are EXPLICIT (previous). There are only 6 hooks to get started with, each one with clear intent & meaning (maybe not so on data)

+1 for the hook renames, they seem more explicit (at least for me)

@yyx990803
Copy link
Member Author

The reason why I think canActivate and canDeactivate are redundant: Simply returning false in canActivate is almost useless, because the user will just be puzzled why the navigation didn't work. In most cases, we want to do something when the navigation should be prevented, for example, redirect the user to the login page:

canActivate (transition) {
  if (!loggedIn) {
    transition.redirect('/login')
  } else {
    transition.next()
  }
}

Now, doing the same thing in activate (or the new onEnter) achieves the same goal, so why even have separate hooks? To me it sounds like a good tradeoff to halve the hooks API surface vs. the little explicitness of a dedicated hook.

@amirrustam
Copy link

Are there any common use cases that would require the need for canActivate or canDeactivate?

If no, then there is sufficient reason to adopt the proposal. I think it's a good goal to enhance the surface API to accommodate the 99% of use cases.

@thelinuxlich
Copy link

love the proposal!

@sylvainpolletvillard
Copy link

onEnter, onLeave, onChange : certainly much more intuitive names 👍
Also I agree with @yyx990803 arguments concerning canActivate and canDeactivate

@blikblum
Copy link

blikblum commented Feb 4, 2016

(Maybe?) Remove canReuse. The same component is always reusable. Handle route change in onChange.

There's at least one use case that i can think for reuse: when you need to prevent deactivate changing to/from a same route with different params

See the example below. It prevents to change route when Person is dirty. Removing canReuse: false i can change from Person 1 to Person 2 regardless of dirty

http://codepen.io/blikblum/pen/EPepaG

@rpkilby
Copy link

rpkilby commented Feb 4, 2016

How drastic are the changes to the underlying code? Would it be possible to provide deprecation warnings for the old hooks before completely removing them?

  • Not everyone reads the issue tracker, so are unable to weigh in on this discussion.
  • Deprecation warnings would provide backwards compatibility and give users a chance to try to migrate to just onEnter instead of canActivate & activate.
  • If it turns out that canActivate really is necessary, then maybe re-add as canEnter?

@yyx990803
Copy link
Member Author

@blikblum yeah, keeping canReuse seems fine to me if there are valid use cases.

@rpkilby we sure can do that, although as a 0.x release I'm not sure if it's worth it to start doing fully backwards compatible releases. Per semver, if you have ^0.7.x in your package.json you will never get 0.8 unless you explicitly upgrade. And if you are doing so, you should be reading the release notes.

@blikblum
Copy link

blikblum commented Feb 4, 2016

@yyx990803 Fine.
BTW. Is there another way of implementing the use case i cited without canReuse?

@rpkilby
Copy link

rpkilby commented Feb 4, 2016

@yyx990803 that's a fair statement about compatibility guarantees. In this case, it's more about prompting the community for feedback before the changes are finalized. Users would still be able to use 0.8.x while discussing whether or not there are any use cases that justify keeping around the canActivate/canDeactivate hooks.

@hartmut-co-uk
Copy link

Hi, regarding the canReuse topic, there's another use case (#474)

Or taking this even further (than the fade in/out effect) - in my case I was experimenting with dynamic routes, e.g. picture a CMS where a user can add new pages with custom slugs.
The routes could look like /content/page1, /content/page2, /content/that-is-a-slug, ...
(well same with /product/:id ...)
Now you have a 'content' component requesting your data from the backend to fetch content by slug / redirect to 404 if not found..

To go with one request only I fetch my data within activate hook, store content to component prop, redirect if not found..

...so without canReuse: false => activate is never called and I can't redirect on 404...
(...and I won't have transition effects)

@posva posva added this to To consider (do not agree with reasoning or could be exposed as plugin, needs more discussing) in Longterm Mar 26, 2019
@posva
Copy link
Member

posva commented May 9, 2020

Closing as this is now outdated and most things are implemented with a different naming.

@posva posva closed this as completed May 9, 2020
@posva posva moved this from To consider (do not agree with reasoning or could be exposed as plugin, needs more discussing) to Done in Longterm May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests