QubitProducts / cherrytree

A flexible nested router.
MIT License
108 stars 20 forks source link

Transition resolve handler added by middleware runs after handler added by application #155

Open blikblum opened 7 years ago

blikblum commented 7 years ago

I have an middleware that attaches a resolve handler to transition using

transition.then(function(){
  setFlag
})

Then in application code

router.transitionTo('parent').then(function(){
  checkFlag
})

checkFlag will run before setFlag.

I hit this issue twice, one when i tried to set a property in router.state to be read after a transition and now when, inside middleware, i set active class for anchors according to href. These issues are showing in the tests and i could overcome them but i foresee actual use cases being affected also

It occurs because transitionTo/dispatch returns before running the middleware

Is there any way to the middleware be notified of the end of the transition before application code?

KidkArolis commented 7 years ago

Aha! I was also frustrated by this. This is kind of just how promises work. The transitionTo promise is the same exact promise that is passed to the middlew are. But this is fixed in the upcoming V3 where middleware can register next/done/error callbacks and transition descriptor is no longer a promise, but plain data.

How urgent is this for you? For now, you could defer the checkFlag with a setTimeout,1. Not elegant but might do the trick in the short term..

On Sat, 26 Nov 2016 at 09:57, Luiz Américo notifications@github.com wrote:

I have an middleware that attaches a resolve handler to transition using

transition.then(function(){ setFlag })

Then in application code

router.transitionTo('parent').then(function(){ checkFlag })

checkFlag will run before setFlag.

I hit this issue twice, one when i tried to set a property in router.state to be read after a transition and now when, inside middleware, i set active class for anchors according to href. These issues are showing in the tests and i could overcome them but i foresee actual use cases being affected also

It occurs because transitionTo/dispatch returns before running the middleware

Is there any way to the middleware be notified of the end of the transition before application code?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/QubitProducts/cherrytree/issues/155, or mute the thread https://github.com/notifications/unsubscribe-auth/AATzWHN77BtRo2zatTz62m3cF3uAW6mbks5rCAJ7gaJpZM4K82v_ .

blikblum commented 7 years ago

No hurry.

This is how i solved in tests: https://github.com/blikblum/marionette.routing/commit/385a150f106b2188007f6dca00acfd20088fc8f5#diff-b999162896fd53962d25a5a16a3d4044R103

Given that is mostly a visual thing there will be no problem. Only if someone queries for active class inside resolve handler. I will leave as is for now.

BTW: many thanks for your work. It allowed me to build a nice router abstraction for MarionetteJS: https://github.com/blikblum/marionette.routing

nathanboktae commented 7 years ago

But this is fixed in the upcoming V3 where middleware can register next/done/error callbacks and transition descriptor is no longer a promise, but plain data.

Interesting. This will align more to other middleware style frameworks. Do the external contracts still return a promise? I hope so as that is very powerful.

It allowed me to build a nice router abstraction for MarionetteJS:

👏 glad to see Cherrytree integrated with more JS frameworks 🎉

KidkArolis commented 7 years ago

The goal of the new version is to simplify - simplify the api, the implementation, remove edge cases such as returning transition in a middleware causing a deadlock, or how redirecting/aborting rejects the transition promise. One of the main ways that's achieved is by not making transition so overloaded, currently a transition is a promise, it holds transition data and it contains methods to manipulate router state - redirect and abort. Instead all those are split out - middleware get pure json data (and any custom attached objects like React components), lifecycle is handled with the different hooks - next/done/error, instead of having to chain on the promise within the middleware, redirect/abort functions are passed to middleware. Anyway, I need to dust that branch off, haven't looked at it recently.

And to answer your question - router.transitionTo() still returns a promise, since it turned out to be very useful in tests, and possibly might be useful in real apps (although I'm a big fan of affecting state via middleware, and using transitionTo() as fire and forget dispatch function). But it's only that - a promise, it doesn't have data or abort/redirect functions attached, since that complicates things. Do you find doing things like router.transitionTo().then() or router.transitionTo().abort() useful? In v3 you'd need to find a way to do that via middleware, e.g. a middleware could be something like

{
  next: (transition, redirect, cancel) => {
    if (!view.willNavigate()) cancel()
  }
}

I'm thinking about cutting a v3 beta version and asking you and whomever else is interested to give it a spin.

nathanboktae commented 7 years ago

router.transitionTo() still returns a promise, since it turned out to be very useful in tests, and possibly might be useful in real apps

Yes I agree fully. It's nice for code outside the router to use it in that manner. e.g. some view that wants to send the user somewhere and do some cleanup only after it succeeds. It doesn't and shouldn't need to know the router internals.

Do you find doing things like router.transitionTo().then() or router.transitionTo().abort() useful? In v3 you'd need to find a way to do that via middleware

if it still returns a promise, then you can chain things via then() right? abort() not working is understandable and we don't use it.

KidkArolis commented 7 years ago

Yes, you can chain and handle errors, etc., it's just a pure promise.

KidkArolis commented 7 years ago

A small update..

Instead of pushing on cherrytree V3 front, I've put together a new router, a spiritual successor to cherrytree - https://github.com/KidkArolis/space-router. It captures the core idea of cherrytree where routes are mapped to data and a single callback is responsible for rendering the app. But it's a much smaller and simpler implementation. Today, where one way data flow and central store approach is popular, there's a lot less need for a middleware mechanism and async routing. Anyway, I'll be trying space-router out in practise and will see how it goes.

I could also bring some of the ideas and code from space-router to cherrytree V3, such as doing away with extra dependencies on location-bar and path-to-regexp, but not sure the time will permit. And not sure it's necessary, cherrytree might be serving it's purpose, though I'd encourage to try out the new simpler space-router and see if it fits the bill. Simpler is often better.

blikblum commented 7 years ago

Nice, i'll take a look.

I could also bring some of the ideas and code from space-router to cherrytree V3

Any chance V3 get out, at least in the current form, without back porting the new ideas from space-router?

nathanboktae commented 7 years ago

Instead of pushing on cherrytree V3 front, I've put together a new router, a spiritual successor to cherrytree

My knee jerk reaction to space router is ugh, yet another javascript library that didn't last 2 years. I'm really frustrated at the hipsterness of the front end community.

But in seriousness looking into space router, while I like it's more lightweight, the single callback is a step backwards in my opinion. The purpose of middleware and the .use is to compose separate concerns together. The preact rendering code you show in your examples won't care about something that will handle authorization, maybe one that checks to see if your session expired, maybe another that checks to make sure a new deployment came out and the user needs to refresh. Sure you can stitch that together in your one rendering function, but it's more burdensome on the user.

I'd also like the V3 concepts come to fruition. Honestly I'm pretty happy with v2 but I see @blikblum point that promises is more limiting then next callbacks that give middleware more control about how they run.

KidkArolis commented 7 years ago

Yeah, that's for the comments, it encourages me to continue on cherrytree.