Implement dispatching iteratively.

[sgomes]

Hello, page.js maintainers! 👋

In applications with a large number of middlewares, the recursive implementation of nextEnter and nextExit become a problem, by generating deep call stacks (2x the number of middlewares, as far as I can tell).

Even where the call stack size isn't directly an issue, the added cruft to the call tree can make debugging and profiling harder.

Recursive (note the many layers of nextEnter):

Iterative (note how there's a single layer of nextEnter now):

There doesn't seem to be a strong reason for these methods to be implemented recursively, so I reimplemented them iteratively.

All the tests work correctly, but please double-check the logic, and let me know if there's anything wrong with it, the coding style, or my commit in general.

Thanks for taking a look! 🙂

[matthewp]

+1 for using a do/while loop!

[coveralls]

Coverage decreased (-0.2%) to 90.256% when pulling d4cc5e4fca33f47e55051b38d426d93f4d606404 on sgomes:master into 043e68cfdea0c8adec24c4e719c4af2c956b96e0 on visionmedia:master.

[paulocoghi]

I cannot check the errors generated on Travis. @matthewp , do you have the permissions?

[sgomes]

@paulocoghi It looks like one of the builds failed during the install phase due to issues with engine-dependencies.

[matthewp]

I think I know why the engine-dependencies thing is happening. I'll fix that. Need to get the code coverage back up too though.

[sgomes]

@matthewp There was a small difference in logic that didn't affect the tests but did route the code through a different path. That's now fixed, and coverage went slightly up, likely due to there being more lines of code overall.

And as a bonus, you now get two do/while loops 😁

[jsnajdr]

What if next is called asynchronously by the handler? Then the new implementation fails to call the next handler in the chain.

Automattic/wp-calypso does the async call when loading a Webpack chunk for a route:

• there is a page( '/settings/*' ) handler that starts loading the settings chunk from network
• after the chunk is loaded, its JS code is executed
• the chunk's static initialization installs page handlers specific for the chunk (page( '/settings/writing' ) etc)
• next() is called and the just-installed handlers are executed

[jsnajdr]

However, I think there is still a chance to make it work. The idea is to detect if next was called synchronously:

let nextCalledSynchronously = false;
wrappedNext = () => {
  nextCalledSynchronously = true;
  ...
};
handler( context, wrappedNext );
if ( nextCalledSychronously ) {
  continueWithIteration();
} else {
  continueWithRecursion();
}

and depending on the result, chains of synchronous next calls can be handled with a loop, and async calls can be handled with recursion as before.

That should still compress the call stacks very significantly as most handlers call next synchronously.

[matthewp]

Great points @jsnajdr. What do you think about this @sgomes?

[sgomes]

Wow, excellent catch, @jsnajdr! Thank you for taking a look!

I had assumed the callback couldn't be called asynchronously, since the docs don't mention that, and it had looked to me that an async call would interfere with the loop because of the early return. But I was wrong; looking at the code closer, the function's closure should have everything it needs to execute correctly. And, well, the fact it's being used that way is the ultimate proof 😁

@matthewp Sounds like I have some further work to do on this PR! @jsnajdr's approach sounds good, so I'll give that a try. I'll also add a test or two for the async behaviour, and validate that they work before and after my changes.

[matthewp]

Perfect collaboration! I'm fixing the issue that is causing CI to fail. Will ping when that is ready.

[jsnajdr]

@sgomes There's another issue where the behavior of the handler chain changes: when a handler executes code after calling next:

page( route, ( context, next ) => {
  preprocess( context );
  next();
  postprocess( context );
}

then the call order changes: with your patch, it's preprocess, postprocess, next. I.e., it's always tail recursion, no matter where the next call is.

[matthewp]

If you rebase with master it should pass CI now.

[sgomes]

@jsnajdr Yes, that's true as well. Darn. If we want to support a scenario where post-processing exists and is meant to be done in order, then I don't think the iterative approach is viable.

I'm not sure how reliable that is, since the post-processing will only occur once all callbacks have been processed (i.e., on the "return trip" for all the nested functions), but it is indeed a breaking change.

Is that approach used in Automattic/wp-calypso?

[sgomes]

If you rebase with master it should pass CI now.

Thanks, @matthewp! I may have to drop the PR, though, given the unavoidable breaking change that @jsnajdr pointed out. What do you think?

I'd be happy to add the async tests in any case, though :)

[sgomes]

@jsnajdr @matthewp Appended a commit that adds support for async callbacks, since I already had it nearly finished before the last comment.

As mentioned before, it doesn't solve the out-of-order issue, so if that's important to preserve let me know and I'll close the PR and create a new one for just the tests 👍

[matthewp]

@sgomes I definitely don't want to break the API in a subtle way. In a future version of page.js when we can drop support for browsers that don't support generators perhaps that can be used to work around the recursion problem and get a better debugging experience, but I suspect that will be quite a ways away.

[sgomes]

@matthewp No worries at all :) I'll create that test PR, then 👍 Thanks for the help so far!