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

Transition Middleware #22

Closed
aldendaniels opened this issue Aug 10, 2015 · 24 comments
Closed

Transition Middleware #22

aldendaniels opened this issue Aug 10, 2015 · 24 comments

Comments

@aldendaniels
Copy link

I'd like to hook into history to do data loading. To do this, I need to be able to pause a transition while data loading is occurring.

I think this can be simulated today using registerTransitionHook() and a custom getUserConfirmation() handler that doesn't actually show a user confirmation dialog. This is of course a hack.

A better solution would look something like this:

let history = createBrowserHistory();
history.registerTransitionMiddleware(function(nextLocation, prevLocation, fnNext, fnAbort) {
   fetchSomeData(location).then(fnNext);
});

The transition would simply freeze until fnNext or fnAbort are called.

If the direction sounds good, then I could submit a PR for this.

@aldendaniels aldendaniels changed the title Programmatically Pause Transitions Transition Middleware Aug 10, 2015
@mjackson
Copy link
Member

I need to be able to pause a transition while data loading is occurring

I don't think you'd actually want to do this. For the user, the page would be non-responsive while you're fetching data (i.e. they would click the back button and nothing would happen for a while).

@aldendaniels
Copy link
Author

@mjackson - this simulates the browser's native behavior - if I click a link on a page, then I'm left looking at the current page until the target page loads.

You would, of course, want to show a loading indicator during the transition phase, to keep the UI from feeling unresponsive.

@gaearon
Copy link
Contributor

gaearon commented Aug 11, 2015

@aldendaniels

We used this approach and it turned out to be a disaster. For example, while data is loading, what does Back button do? Does it cancel transition? If not, how can you cancel a transition that's taking too long (e.g. bad mobile connection). So does Back cancel pending transition? But that looks very weird on mobile, where "Back" is a slide gesture.

My advice: change URL right away. Changing it later = bad UX. I've learned it hard way.

@aldendaniels
Copy link
Author

@gaearon - Thanks for weighing in.

The browser natively does exactly what you're describing as being problematic on mobile - if you slide to go back during a pending transition then you slide "back" onto the same page you were on (cancelling the transition). Since this is native behavior, why not simulate it?

Are there other complications I'm missing for an SPA?

@mjackson
Copy link
Member

What don't you like about doing something like this, @aldendaniels?

history.listen(function (location) {
  fetchTheData(location, function (data) {
    renderThePage(location, data);
  });
});

The current implementation of transition hooks is minimal on purpose. It only addresses the specific use case of confirming transitions away from a route. If you need to run some logic after a user hits a route, you can already easily do that.

@aldendaniels
Copy link
Author

There are a couple of issues:

  1. Changing the URL immediately is (IMO) an anti-pattern. Native browser behavior, Ember (I think) and popular SPAs like Gmail and GitHub do it the other way.
  2. Hitting the back button will reload data for the previous view, instead of canceling the transition. Again, this feels like an anti-pattern.
  3. I'm using this with ReactRouter, which expects a history object, so I can't just call renderThePage(), I'd need some kind of proxy history object. Actually, the proxy history object is a decent option, but it would have to duplicate a lot of logic that's already built into history.

The middleware layer provides an extensible hook for any kind of async transition operation, whether it's data loading, saving changes, or prompting a user before navigating away. And it's not coupled to React or React router, so it can be used (and tested) in any context.

@mjackson
Copy link
Member

@aldendaniels You make some great points :)

Changing the URL immediately is (IMO) an anti-pattern

There are actually two subtly different use cases here: programmatic and "manual" transitions. When you transition programmatically (i.e. using history.pushState and/or history.replaceState) then yes, we have the chance to defer updating the URL until data is loaded. However, when the user hits the back/forward buttons the URL always updates immediately (at least on desktop), regardless of whether or not the page is loaded.

Since there are two different behaviors here, I chose to go with the latter so we're always consistent. However, I'm hesitant to say that I know this is always the right decision because there is currently an unresolved issue in the code where we 1) update the URL, 2) cancel the transition and then 3) need to put the URL back (see #8 for more discussion). So you may have a point here and we may need to figure out how to support both behaviors, assuming we can't resolve that issue another way.

Hitting the back button will reload data for the previous view, instead of canceling the transition

Doesn't that completely depend on how fetchTheData works? Many client-side apps these days employ a caching layer that lets them request the same data without incurring extra network requests. For example, your data fetching layer could be smart enough to know "I've already seen this location object. Just return whatever we fetched the last time".

I'd need some kind of proxy history object

That's exactly the approach I'm advocating here. To be more clear, you don't have to give the <Router> a history object. Instead, you can give it a location:

history.listen(function (location) {
  fetchTheData(location, function (data) {
    React.render(<Router location={location}>...</Router>, node);
  });
});

You could pretty easily wrap this up in your own history object.

function createDataFetchingHistory(options) {
  var history = createHistory(options);

  function listen(listener) {
    return history.listen(function (location) {
      fetchTheData(location, function (data) {
        listener(location, data);
      });
    });
  }

  return {
    ...history,
    listen
  };
}

var history = createDataFetchingHistory(...);

history.listen(function (location, data) {
  // wahoo!
});

@aldendaniels
Copy link
Author

Thanks for the detailed response! There's some good stuff here.

when the user hits the back/forward buttons the URL always updates immediately (at least on desktop),

When I manually click a link (testing Chrome desktop on mac) then the URL does NOT update immediately - it waits for an initial response from the target page. With back/forward buttons the URL does appear to update instantly, but the the browser serves a cached version of the page so the entire page load is instant.

Many client-side apps these days employ a caching layer that lets them request the same data without incurring extra network requests.

Yes, but a custom caching layer shouldn't be a requirement for this. If an in-progress transition is cancelled via the back-button, then no transition occurred so no data loading should occur. Hitting the back button during an in-progress transition should render the transition a no-op.

To be more clear, you don't have to give the a history object. Instead, you can give it a location:

Ooh, nice, I did not know this. This makes implementing my own middleware layer outside of history much easier. Nonetheless, I can't replicate the desired behavior fully without access to history internals via something like a middleware layer.

Imagine this scenario:

  1. User starts at url /route0/
  2. User navigates to /route1/
  3. Data loader fetches data for /route1/
  4. Before data loading is complete, user navigates to /route2/
  5. User hits the back button

What should happen is that the user is taken back to /route0, because the transition to route1 never happened - it was interrupted. What's actually going to happen is that the back button take the user back to /route1/ because it was synchronously added to history.

I don't see a clean way around this without a middleware layer built into history.

@aldendaniels
Copy link
Author

UPDATE - Added missing step "5" to my scenario above:

User hits the back button

@agundermann
Copy link
Contributor

I think you can manage that scenario by redirecting push calls to replace if there is a pending transition.

I don't understand the need for a middleware API though. Wouldn't it be enough to include the functionality in the onChange listener? Something like

history.listen((location, performTransition) => {
  renderSpinner();
  loadData(location, (data) => {
     performTransition();
     // URL is changed now
     render(location, data);
  });
});

With the listen callback being guaranteed to be called immediately after a push/replace, one could manage pendingLocation or renderedLocation by himself. The middleware API could be built on top of it as far as I can tell.

@aldendaniels
Copy link
Author

@taurose - I think building the middleware layer on top of history would require rewriting most of createHistory.js because you'd have an alternate view of reality with a different history and different back button behavior.

...so yes, I could intercept transitions, maintain my own separate source-of-truth history object, and map POP and PUSH events to REPLACE when they intercept pending transitions. But this is a lot of effort duplication.

HistoryJS already has the concept of a pending transition - used to support prompting users before navigation. Middleware simply extends this concept to support other use-cases by making transitions interruptible. The added complexity seems minimal to me and the gain seems significant.

This said, I don’t seem to be striking a chord here and you’re right, I can accomplish what I need in other ways.

@mjackson - Re-read your post in the light of day. Realized that I'd missed the import of what you were saying about manual transitions via back/forward buttons being immediate by necessity.

It’s true that the URL will update immediately when the user navigates via the forward/back button. This doesn’t mean (per-se), however, that the transition needs to be treated as synchronous. You can still treat the transition as “pending” after the URL has changed, thus getting the desired back-button behavior.

@agundermann
Copy link
Contributor

I think you misunderstood my proposal. You wouldn't have to keep your own history stack or redirect PUSH/REPLACE. Unless you call performTransition, the URL wouldn't be changed, just like in your proposal. You'd only have to manage concurrency yourself to avoid rendering or fetching data for superceded locations.

var middlewares = [];
var addMiddleware = (middleware) => middlewares.push(middleware);
var removeMiddleware = ...;

var pendingLocation = null;
var curLocation = null;

history.listen((location, performTransition) => {
  pendingLocation = location;
  async.series(middlewares, (middleware, next) => {
    if (pendingLocation !== location)  ...
    middleware(location, curLocation, next);
  }), (err) => {
    if (pendingLocation !== location) return;
    pendingLocation = null;
    performTransition();
    curLocation = location;
    render(location);
  });
});

@mjackson
Copy link
Member

I think you can manage that scenario by redirecting push calls to replace if there is a pending transition.

There's probably still some work to be done in the transitionTo function to support this, @taurose. Perhaps instead of throwing we should just change the action of the nextLocation to be a replace there...

@aldendaniels
Copy link
Author

Sorry @taurose, you're right, I had misunderstood your proposal. Thanks for elaborating.

And to @mjackson's point - this does add the requirement of interruptible transitions - but that's not a bad thing IMO.

I think that the design you're describing is (or at least could be) a middleware layer - it's just a different API. I'd envision this working just like Express' API, where listeners are evaluated serially:

// Verify auth.
history.listen((location, fnNext) => {
  verifyAuth()
    .then(fnNext)
    .catch(() => fnNext({  // Redirect - Downstream listeners will not be called.
      path: '/login',
      params: {next: location.pathName}
    });
});

// Load data.
history.listen((location, fnNext) => {
  fetchData().finally(fnNext);
});

// Render.
React.render(document.body, 
  <Root> // Shows top-level loading indicator/overlay.
    <Router history={history} />
  </Root>
);

Is this what you were envisioning?

@aldendaniels
Copy link
Author

And of course you could still use this to replace the current registerTransitionHook() stuff:

history.listen((location, fnNext) => {
  showCustomComfirmPrompt('Are you sure?', fnNext); // false aborts
});

@agundermann
Copy link
Contributor

Almost :) . I actually had a single listener in mind, so you would write something like this

history.listen((location, performTransition) => {
  confirm(location)
    .then(checkAuth)
    .then(loadData)
    .then(performTransition)
    .then(render);
});

Yeah, it would be pretty similar to the middleware API. One advantage I see is that you have the ability (or at least it's clearer how to) cancel ongoing async operations asap since the callback can be guaranteed to always be called immediately. Also, it's not opinionated about when to change the URL and how to run async operations (serial vs parallel).

@aldendaniels
Copy link
Author

Hmm. I agree that a single listener is all that's needed.

If you do this though, you'll want to lock it down so you can ONLY have one listener - which is different from today. Otherwise, what happens if you do register multiple async listeners? Does the first one win?... or the last one?

If you're going to support multiple listeners, then I think serial execution is a good approach - and one with strong precedence (Sinatra/Express).

Also, supporting multiple listeners has the advantage that its easy for multiple 3rd-party libraries to process requests without stomping on each other ...again, like the myriad of Express middleware available.

Checkout how visionmedia's Page.js library does this:

page(path, callback[, callback ...])

Defines a route mapping path to the given callback(s). Each callback is invoked with two arguments, context and next. Much like Express invoking next will call the next registered callback with the given path.

Context

Routes are passed Context objects, these may be used to share state, for example ctx.user =, as well as the history "state" ctx.state that the pushState API provides.

Personally, I like this approach.

@aldendaniels
Copy link
Author

Also, it's not opinionated about when to change the URL and how to run async operations (serial vs parallel).

I don't think supporting multiple listen() calls makes this more opinionated - you can still register a single listener and parallelize internally.

@aldendaniels
Copy link
Author

So were are we with this? I'm happy to update my PR with whatever API is decided on... and I can work around the existing API if the decision is to keep the API unchanged. Whatever the outcome, I'd like to move forward (or not) as soon as there is consensus.

@mjackson
Copy link
Member

I can work around the existing API

Let's go with this for now, since I'm still not quite able to see where our API falls short.

Often when I'm working around someone else's API limitations, it helps me solidify my thinking about how that API needs to be changed so I can do what I need to do more easily. If that happens here, please do follow up and let us know.

Closing for now, but I'm happy to re-open later if you feel you'd like to continue this discussion, @aldendaniels.

@aldendaniels
Copy link
Author

@mjackson - fair enough. I'll be using your suggestion:

history.listen(function (location) {
  fetchTheData(location, function (data) {
    React.render(<Router location={location}>...</Router>, node);
  });
});

The drawback is that the back button will misbehave, but I can live with that for now.

mjackson added a commit that referenced this issue Aug 21, 2015
This should also help out with @aldendaniels' use case in #22.
@mjackson
Copy link
Member

Hey @aldendaniels - just wanted to give you an update here. After thinking this through for a while and working to build the new react router API on top of this lib, I decided to make transition hooks async as you suggested here. The work was done in ae8dd6f and should be published in a minor version bump (since the API is backwards-compat, i.e. you can still return if you want).

@aldendaniels
Copy link
Author

@mjackson - excellent, thanks for the heads up!

From a quick glance at the change, it looks like you're not running the transition hooks for the very 1st transition (e.g. the first time .listen() is called). This is problematic for the data-loading/authentication use case.

@aldendaniels
Copy link
Author

@mjackson Just checking in again. See my previous comment. Would love to know your plans hear - I'm also happy to make a PR.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants