faceyspacey / redux-first-router

🎖 seamless redux-first routing -- just dispatch actions
MIT License
1.56k stars 143 forks source link

Make onBeforeChange honor promises #90

Closed jzimmek closed 6 years ago

jzimmek commented 7 years ago

The onBeforeChange function is not promise aware. This makes it very hard to involve any async operation to decide whether we cancel the route (through dispatching a redirect) or not.

Would be great to have a onBeforeChange function which will honor:

  1. a promise dispatched from within onBeforeChange (like redux-thunk)
  2. or will await a promise returned from onBeforeChange

And will evaluate the "skipping" after the promise resolved.

This will enable a lot of possible use-cases like authentication (an existing cookie/jwt does not guarantee that it is still valid), near real-time stock/reservation checks, lock/conflict handling etc.

Looking into the code it seems feasible without a lot of change.

If this is something you would consider as a PR I would start working on it.

faceyspacey commented 7 years ago

I'd love the PR. But I think it would be more challenging than it initially seems since it runs in Redux middleware which is synchronous. There is a workaround. This exact issue was actually closed 12 hours ago:

https://github.com/faceyspacey/redux-first-router/issues/85

I describe the workaround in the above issue, but it likely will skyrocket the complexity of implementation, which opens the doors to potential other problems + edge cases. It almost makes it a better option to just rewrite redux to have an async middleware chain (which is basically copy/paste code from other async middleware chain implementations; makes me wonder why Redux itself has yet to take this route).

...Glad you noticed the concept of an existing cookie/jwt as the primary route to take. But when would an existing cookie/jwt not be valid really be an issue. If a user logged out, you can synchronously determine there isn't a valid cookie/jwt and redirect to a login page. And otherwise, on revisiting the app, the cookie/jwt is best handled when the store is configured.

For the realtime-stock preservation checks etc, or really anything, do you really want the user trying to visit a URL and it not showing immediately. Perhaps that stuff is best handled in a thunk or in onAfterChange, and the corresponding UI elements do not show until validated. In short, usually when you visit a URL you expect it to change immediately.

Anyway, I'm attracted to the idea, but it boils down to redux middleware being sync, and requiring extensive workarounds/hacks to go through the middleware multiple times to fully resolve the route change. A custom action creator might even be needed to relay that onBeforeChange is resolved (but maybe not).

faceyspacey commented 7 years ago

so off the top of my head, here's how i see the implementation:

faceyspacey commented 7 years ago

the ID stuff may or may not be needed. It could be implicit if somehow we guaranteed the user didn't click around and trigger such a scenario again. We'll cross that bridge when we get to it. There's probably a simple semi-automatic way to handle it. So we just keep it in the back of our heads.

faceyspacey commented 7 years ago
faceyspacey commented 7 years ago

in short onBeforeChange handling promises sounds "promising." If you're up to the task, I'd say there is a 75% chance you/we end up with something great. But there's always a chance it ends up being a waste of time and for some weird reason we can't do it or incorporate it. The devil is in the details, as always.

jzimmek commented 7 years ago

@faceyspacey first of all, thanks a lot for the quick and very detailed response.

i will think about all those point. you are right, i was not aware that all is happening inside the middleware. will dig around in the code and let you know when i have something worth to discuss about.

faceyspacey commented 7 years ago

Cool. One other thing: SSR. Basically in ssr, a thunk is returned and you do just await thunk(store) before returning a response. The reason is again because the redux middleware is sync. So we give the user the tools to handle asynchrononicity.

Now, with onBeforeChange also being async--we need to either make it so the thunk also addresses that or return yet another item that is awaited on. I'd hope it can be handled in the thunk. Currently, you simply await it, without worrying about the return since it independently dispatches some new state to the store. So perhaps we could have a returned object that contains what was dispatched/resolved in onBeforeChange.

silouanwright commented 7 years ago

In the RFR example on codebox you redirect the user to the login page telling them their login didn't succeed. Most sites nowadays show you a login modal, and when you submit, they do some evaluation before punting you over to the login page. If this is what this issue covers, I'm a fan of it, as opposed to preventDefaulting every anchor and doing some custom logic if I want to show a loading state before navigating to that link.

silouanwright commented 7 years ago

I almost think it would be super slick if there was a onBeforeThunk on the routes themselves. I feel like on a lot of the better sites, an ajax request is made and the navigation is delayed till after the request returns, showing a loading state on the view you're currently on. the onBeforeChange is useful for the login paradigm, but not for what I'm referring to. Or is there a better approach? It's not the end of the world as again we hijack the link and dispatch to the correct route later, but imagine if that was integrated into RFR

faceyspacey commented 7 years ago

U use the route action type in your reducers. Less callbacks. Less dispatches. Less actions. It's al already there. The route action is the "setup action" to trigger loading...

silouanwright commented 7 years ago

Thanks (and thanks for replying over in your chat). You don't have to reply to this but I'd like to explain the motivation as I know this might be a tough problem to address.

When we’re navigating through a traditional website rendered from the server, the browser shows it’s own loading state and continues to show the current page until it receives the response and then updates the url.

When we’re talking about SPA’s, one of the downfalls is that when you have a routing system, you’re forced to manage the next page before the response has even returned. That means if you have 3 components on the next page all awaiting data, you have to make sure all of them have loading states. This is exhausting for the developer to maintain, and it’s also exhausting for the user. As a user, I don’t want to click a link, then get an unusable page, and then scan the page for all the modules to complete before using it. I just want to be notified when the page is done.

Also on the server, if you attempt to render a page but you get a 500, you don’t even attempt to render the page you were trying to navigate to. For the SPA routing, if you just navigate the user through to their link and find out you won’t be getting valid data… uh oh! Now you have to start thinking about redirecting the user back to where they were at, as the current page isn’t usable and isn’t going to be. If you keep the user at that route, now you have to deal with stopping the loading states on the components and dealing with that as well.

The only real reason for using legit URL’s is 2 reasons. 1, for the users convenience. We want them to be able to open links in a new tab, restore from browser crashed session, share links online or with their friends. Ideally we want to fetch the payload asynchronously as it’s a faster and better experience but we want to give them a way to circumvent that, we want better SEO, etc. The 2nd reason, is all the RFR goodness:

Some of this may be wrong... I'm just getting acquainted with the routing world... but I figured I should explain the use case behind something like this.

hedgepigdaniel commented 7 years ago

Suppose you did roughly as follows:

  1. Link an action to RFR to update the URL (this part will still happen immediately) but don't use that action or the RFR location sub store to update your UI.
  2. Provide a thunk in the routesMap for each of your routes, that will start the process of loading the data for the components on the next page. When its done, possibly asyncronously, it can dispatch a second action indicating that the data is ready or that fetching it failed, or some other situation.
  3. Your own location reducer can listen to this action and actually switch the UI to the new page. Alternatively on failure you can probably dispatch a different action to restore the previous page.

Wouldn't that implement the important parts of what you want?

faceyspacey commented 7 years ago

yea, that sorta stuff would handle it, but it's less than ideal.

That said, and I've been planning on posting here some big news soon, so i'll do it now even though it's not ready. In the next version of RFR/Rudy, we're gonna have a full support for promises across all callbacks. There will be multiple global callbacks in options and identical ones per route. There will be new callbacks as well. And they will all be able to handle promises.

This actually presents a lot of challenges. That said, the result is our own middleware mechanism of our own. It will be koa's promise-based middleware mechanism. It's already built and working very well, though a lot more still had to be done. Supplying middleware will be optional, and everything will work pretty much as it does now, plus new callbacks. And then if you want to further customize the pipeline, you can provide an array of middleware.

I've been working on this for a while now, I'm gonna need another week or 2 to get it out. When it's out, this is the first thread I'm gonna share it on. See you guys soon.

Vincz commented 6 years ago

Hey James! Any news about this ? I'm facing a similar issue. I have a redux middleware, trying to do async stuff and delaying the call to next(). The purpose of this middleware is to keep a redux state in sync based on a route payload. So for example, if my middleware found a key city_id in the route's payload, it loads the city (async operation) and store it in the store. And I need to do it BEFORE the route action occurs. It works well on the client (as the action resolution is just delayed), but server side, the app is rendered before the resolution of the dispatch and the rendering failed (as the redux middlewares are not async). I solved it by storing my promise in the state and wait for it resolution server side but I don't like it. I guess this is the kind of problem handled by the next big release ? :) Any news about it ?

Have a wonderful day!

faceyspacey commented 6 years ago

hey @Vincz good to hear from you! I got derailed the last month with the holidays and a big move to another state. I'm back 100% on getting Rudy out and it will solve all such problems. It has a middleware API of its own, giving you lots more control. I'll be sure you hear from me when it's out, rest assured :) I'm aiming for the next 1-2 weeks.

hedgepigdaniel commented 6 years ago

Just flagging that the rewrite is available if anyone wants to try it out: https://github.com/ScriptedAlchemy/redux-first-router/tree/rudy-respond

Currently there is no documentation and it hasn't been tested much, but the example/boilerplate works and all the route callbacks support promises :).

ScriptedAlchemy commented 6 years ago

Yep, docs will be worked on, so is the codebase. But we are looking for people to play around with the demo - currently stable

ScriptedAlchemy commented 6 years ago

Closing this as the project enters LTS -> Rudy, the sucessor will have a new repo under the respond organization

eniolopes commented 5 years ago

Hey, looks like development on Rudy has stopped and this is the way to go moving forward? If this is the case, any chance we can get that async behaviour going for the onBeforeChange function? I'm happy to submit a PR.

ScriptedAlchemy commented 5 years ago

James actually came back from a year long break. We anticipate a release before the end of April

ScriptedAlchemy commented 5 years ago

Ill accept a pr if you are able to make one tho.

hedgepigdaniel commented 5 years ago

Development on Rudy has not stopped - see here: https://github.com/respond-framework/rudy

The async callbacks are already working. You can return a promise from thunk, onBeforeChange, etc and they will be waited for.

faceyspacey commented 5 years ago

@eniolopes Rudy will be released as a separate package, preserving the way RFR has always run, while providing a new avenue for a lot more capabilities.

eniolopes commented 5 years ago

Cool, I'll migrate to Rudy, thanks for the responses :)

ScriptedAlchemy commented 5 years ago

Follow us on Twitter - same user handles. I’ll post updates