Skip to content

RFC for Hook/Event for Transitioning During a Transition#126

Closed
nickiaconis wants to merge 3 commits intoemberjs:masterfrom
nickiaconis:redirect-notify
Closed

RFC for Hook/Event for Transitioning During a Transition#126
nickiaconis wants to merge 3 commits intoemberjs:masterfrom
nickiaconis:redirect-notify

Conversation

@nickiaconis
Copy link

@nathanhammond
Copy link
Member

Yo dawg, I heard you liked transitions so I put an event in your transition so you can monitor transitions while you transition.

/cc @stefanpenner @tomdale

@tomdale
Copy link
Member

tomdale commented Mar 21, 2016

Love it. Seems like an obvious win.

willChangeTransition: hookForAllTransitions,
});

Router.on('willTransition', callbackForAllTransitions);
Copy link
Member

Choose a reason for hiding this comment

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

There are a few examples in this RFC that suggest:

const Router = Ember.Router;

Router.on('stuff', function() {
  alert('changing!');
});

I think I get what you mean here, but I didn't think you could call .on on the Router class itself (I thought it only works in instances). Demo here: http://rwjblue.jsbin.com/koqofoc/edit?html,js

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Also, willTransition and didTransition fire as actions:

http://jsbin.com/nemuzeteju/1/edit?html,js,output

It is proposed that an action be added for willChangeTransition? Is that possible to add, since the router is in transition (I'm not crazy familiar with the code)?

Copy link
Member

Choose a reason for hiding this comment

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

Ya, so they fire as method calls and events on Ember.Router instances, and as actions on Ember.Route instances. We should do the same pattern here with the new ones.

Copy link
Author

Choose a reason for hiding this comment

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

@rwjblue Good point about the instance/class mixup. Will fix.

On the method/event/action side, I'm a little confused. Are you saying willChangeTransition should exist for Routes as well as the Router? I'm of the opinion it should not.

Copy link
Member

Choose a reason for hiding this comment

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

@nickiaconis - If the idea is to augment the existing willTransition / didTransition hooks then it stands to reason that the new hook should fire the same ways that the ones it is augmenting does. Since the existing hooks for willTransition and didTransition fire as both events and methods on the Ember.Router instance and fire as actions on Ember.Route instances I would have assumed that the new one would also.

I suppose that I don't care a ton that the new willChangeTransition follows that same pattern, but we should have a very specific reason for deviating.

@mixonic
Copy link
Member

mixonic commented Apr 8, 2016

I've added this to the agenda for next week's core meeting 😄

@mixonic
Copy link
Member

mixonic commented Apr 15, 2016

@nickiaconis we talked about this today. I believe there is some feedback from @wycats coming re: the name and the pedagogical section. I'll punt that to him.

This RFC doesn't mention adding willChangeTransition (or a better name) as an action. For example in the guides the first example of using willTransition is as an action, not a router event. This should be added, and the timing of the action (on the pre-redirect route tree or post-redirect) would be part of that as well.

Thanks for your continued work on this, the whole team is enthusiastic about keeping you moving and landing the feature!

@nickiaconis
Copy link
Author

@mixonic I do not believe this hook/event should be implemented for Routes. I believe it should only be implemented for the Router. Here's a look at the lifecycle of a transition to explain why:

  1. A transition is initiated from /index to /messaging/index
    • let's say our router map looks like [/index, /messaging/index, /messaging/:thread-id]
  2. willTransition is triggered on the /index route
    • this provides the /index route a chance to do any manual clean up it needs to do
  3. willTransition is triggered on the Router
    • this provides the app a chance to run pre-routing logic
    • perhaps this includes fetching requests for the /messaging/index route
  4. The hooks of /messaging/index (model, redirect, etc) are called, one of which causes a redirect to /messaging/:thread-id
    • thread-id is the ID of the most recently sent/received message thread
  5. willChangeTransition is triggered on...
    • what route would it be triggered on?
      • didTransition has not been triggered on /messaging/index so the route should not have set up anything that needs to be torn down manually
      • willTransition was already triggered on /index, so it doesn't make sense to trigger it there
      • we haven't touched /messaging/:thread-id yet, so it doesn't make sense to trigger it there
    • what purpose would triggering it on any of the candidate routes provide?
  6. willChangeTransition is triggered on the Router
    • this provides the app a chance to run pre-routing logic
    • perhaps this includes fetching requests for the /messaging/:thread-id route
    • (does this sound familiar? it's the same thing we did when willTransition was triggered on the Router)
  7. The hooks of /messaging/:thread-id (model, redirect, etc) are called
    • a message is loaded and prepared to be displayed to the user
  8. didTransition is triggered on the /messaging/:thread-id route
    • this provides the /messaging/:thread-id route a chance to do any manual setup it needs to do
  9. didTransition is triggered on the Router
    • this provides the app a chance to run post-routing logic
    • perhaps this includes generating a tracking event, and submitting it to the server

Step 5, triggering willChangeTransition on a Route, appears to have neither a purpose nor a straightforward answer for which Route receives the action.

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2016

I definitely appreciate the effort and write up here @nickiaconis. Thank you for being thorough and passionate. 😸


To your question:

what route would it be triggered on?

The same route that willTransition was called on. Assuming that willTransition is used to queue up information (say in a service) for a future didTranstion action to use for reporting purposes, giving it the opportunity to change that information is no different than wanting access to willChangeTransition in the router itself.


I am very 👎 on adding a new event that seems similar to willTransition in many ways that doesn't follow the same conventions as willTransition / didTransition.

@mixonic
Copy link
Member

mixonic commented Apr 15, 2016

@nickiaconis during step #5 what is the active route? The transitioned-from route (/index above) or the redirecting route /messaging/index? I expect whichever is active would receive the action, since all we need to do here is send the action to the router and it will dispatch it (to the active route) accordingly.

@nickiaconis
Copy link
Author

Assuming that willTransition is used to queue up information (say in a service) for a future didTranstion action to use for reporting purposes, giving it the opportunity to change that information is no different than wanting access to willChangeTransition in the router itself.

@rwjblue The use case you've described spans routes, as I believe all use cases of willChangeTransition do.

Concerns that span routes belong at the Router level.

Implementing route-spanning concerns at the Route level increases the coupling between the involved routes. I think it is the framework's responsibility to not provide primitives that will lead developers into anti-patterns.

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2016

Implementing route-spanning concerns at the Route level increases the coupling between the involved routes. I think it is the framework's responsibility to not provide primitives that will lead developers into anti-patterns.

Actions all bubble, it is extremely common to have a single willTransition / didTransition action in the top level route (application). At which point this is very similar to events on the router.

I think it is the framework's responsibility to not provide primitives that will lead developers into anti-patterns.

I absolutely agree! It is also the frameworks responsibility to be internally consistent.

@nathanhammond
Copy link
Member

I don't feel particularly strongly about willChangeTransition existing or not existing at the Route level. We can implement that without tremendous difficulty, throw it into the canary channel and see what people think. I propose adding it to the RFC and then we can adjust given feedback while in canary.

@nickiaconis Seems like the only open question is what the active route should be in your step five above?

@nickiaconis
Copy link
Author

@nathanhammond Sounds to me like @rwjblue already answered that.

The same route that willTransition was called on.

I'll update the RFC to include this at the Route level.

@kanongil
Copy link

kanongil commented Jul 7, 2016

I'd just like to chime in that I would really like to have this hook available.

I have a question about how this interacts with transition.abort()? Eg. is the old transition aborted before or after the willChangeTransition event? And what happens if the willChangeTransition transition is aborted immediately? Would it provide an opportunity to cancel the change transition, continuing with the original transition?

Also, how is the recursiveness handled when a new transition is triggered during the willChangeTransition event?

@nathanhammond
Copy link
Member

@kanongil Inside of the Router there is only ever one "active" transition. Any time that transition changes willChangeTransition would be triggered. It is possible to use this as a primitive to adjust the transition at that point, but note that it would also trigger willChangeTransition and it'd be easy to accidentally throw yourself into a loop.

This proposal is not implemented with the DOM model where something like e.preventDefault would prevent a change. It simply fires when a transition does change so that you're able to track the path through the transitions. It is not designed as a hook for modifying the transition itself. That should be done inside of the hooks for the route which needs to adjust the transition rather than consolidation into some router built on top of the Ember router inside of the willChangeTransition handler.

@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2016

@nickiaconis - Can you review the recent updates to the Router Service RFC and see if the events proposed there will meet your needs?

@nickiaconis
Copy link
Author

@rwjblue - routeWillChange should cover my use case for ember-prefetch. 👍

It will be more difficult to distinguish between a normal transition and a redirect, but should still be doable. This was not a need I saw personally, but others may need it. Tracking is the first use case that comes to mind.

The Router Service RFC's events' names are much better than willChangeTransition, but I suppose naming is easier when you are at liberty to — in fact, when you must — diverge from pre-existing names. 😛

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants