Deprecate re-routing through string parameter to `next`

[ghermeto]

Today

The current next API for route handlers is the following:

next()

• goes to the next handler in the chain (if any)

next('fooRoute')

• re-routes to the route chain that resolves for 'fooRoute'

next(1) (or next({}))

• sends an InternalServerError

next(new Error('boom'))

• returns the error (if it has the statusCode property), otherwise converts it into a String and wraps in an InternalServerError (same as passing it a number as the first parameter)

Proposal

Deprecate the API next('fooRoute') and its async/await equivalent. Instead, use an explicit re-routing mechanism similar to what was proposed by @mmarchini:

  server.get('/one', async (req, res, next) => {
    req.routeTo('getTwo');
    next();
  });

  server.get('/two', async (req, res, next) => {
    res.send('two');
    next();
  });
  

The method .routeTo() will set a re-route flag, that will be read after the handler finished and it would re-route at that point without relying on the error chain.

Since setting the flag is synchronous, this approach also works for async functions:

  server.get('/one', async (req, res) => {
    req.routeTo('getTwo');
  });

  server.get('/two', async (req, res) => {
    res.send('two');
  });

This proposal keeps the functionality intact while removing a potential problem for the async/await DevEx.

Thoughts?

cc/ @cprussin

[cprussin]

I don't see any good reason to support re-route. routeTo is just goto in disguise.

Other than that 👍

[ghermeto]

actually this seems to be a non-documented feature. I can't find documentation of the re-routing in the official docs...

[ghermeto]

cc/ @restify/current-core

[mmarchini]

IMO we can start by deprecating re-routing, and later if we want to explore alternatives we have that option too.

I somewhat agree with @cprussin that routeTo can act as a goto and the current routing mechanism can act as one even more. My proposal was slightly different though: instead of having a "rerouting" mechanism, introduce a method to call another route. That would give more versatility and would allow rerouting implementation in userland. It would also looks more like a function call or a inter-process call than a goto. OTOH the same use cases could be achieved by refactoring routes in the application, so that's not necessarily a good idea either.

[cprussin]

It would also looks more like a function call or a inter-process call than a goto.

In general I don't think restify should have any business providing features that solve problems that are already solved by language-level features. In this case, that language-level feature is function calls. There's nothing about calling routes that really has anything to do with routes or restify.

Even if a route was written with CPS and you want to call it from async/await or vice versa, Node provides callbackify and promisify. I don't think any of this is (or should be) a Restify problem.

So I'm a big vote in favor of what you said @mmarchini -- deprecate first, revisit if and only if use cases come up that are truly restify-specific problems.

[ghermeto]

Ok, agreed. I will start with deprecating. If we find that people use and need this functionality we can implement it in a non-breaking feature.

Now we need to align with happens when you return a value from an async function and when you pass a string to next.

I know that @DonutEspresso would prefer to have a uniform API between callbacks and async/await, but I think they are not the same... I strongly believe the return of async function should be considered the equivalent of res.send, while any value passed to next should be treated as an Error.

The two experiences are not exactly the same, but they provide a better DevEx and it is familiar to users (expected behavior).

Thoughts?

[cprussin]

I strongly believe the return of async function should be considered the equivalent of res.send, while any value passed to next should be treated as an Error.

100% agreed on any value passed to next being an Error. That is the standard signature for CPS in node and violating that would make promisify and callbackify not work as expected (although it's possible to attach custom implementations to functions, but it's a pretty gross and magical tool).

I have less opinion about the return value of the async function. Node is not purely functional and restify is a very imperative & non-declarative API, it feels unexpected to have this bit behave more FP/declarative. Not saying it's a bad thing, just that I wouldn't anticipate that behavior of restify unless I read it in docs.

[cprussin]

@ghermeto Also something to keep in mind--the typical error-first Node CPS equivalent to returning a value in an async function or resolveing a value in a Promise is not to pass the value to the continuation as the first argument, but rather to pass it as the second argument. So the equivalent of:

const someFn = async (req, res) => 'foo';

in typical Node CPS is:

const someFn = (req, res, next) => next(undefined, 'foo');

Thus, I think that implementing a return value from the async function:

1. Is totally feasible to do in a way that you maintain consistency with the CPS API
2. Does not require any breaking changes to maintain that consistency
3. Is completely orthogonal from implementing async/await support
4. Is also totally orthogonal from deprecating re-routing

[cprussin]

Just one more note about this but I think that the implementation we choose in Restify in all ways should be identical to me writing:

app.use(callbackify(async (req, res) => { ... }));

Which already totally works and pretty much behaves exactly as I would expect, and I really have never had a problem doing this except the boilerplate of writing callbackify when interfacing with Restify.

I think any way in which Restify does not behave like that would constitute unexpected behavior from the language/Node platform perspectives.

In fact, if Restify simply implements the conversions by actually using callbackify and promisify, this should all happen for free and there shouldn't be much reason to even have to think about CPS vs async/await APIs.

[ghermeto]

I have less opinion about the return value of the async function. Node is not purely functional and restify is a very imperative & non-declarative API, it feels unexpected to have this bit behave more FP/declarative. Not saying it's a bad thing, just that I wouldn't anticipate that behavior of restify unless I read it in docs.

For people using other web-frameworks with async/await support, like Hapi or Fastify this API should feel familiar. And I can see how Hapi API does not fit the description, but worths mentioning that is a common pattern in the ecosystem.

@ghermeto Also something to keep in mind--the typical error-first Node CPS equivalent to returning a value in an async function or resolveing a value in a Promise is not to pass the value to the continuation as the first argument, but rather to pass it as the second argument. So the equivalent of:

const someFn = async (req, res) => 'foo';

in typical Node CPS is:

const someFn = (req, res, next) => next(undefined, 'foo');

Thus, I think that implementing a return value from the async function:

1. Is totally feasible to do in a way that you maintain consistency with the CPS API
2. Does not require any breaking changes to maintain that consistency
3. Is completely orthogonal from implementing async/await support
4. Is also totally orthogonal from deprecating re-routing

Very good point! It is indeed not breaking the API at all and it is an orthogonal issue, which we should discuss on a separate issue/PR.

Ok, new plan:

Now:

1. deprecate re-routing;
2. next('foo') is considered an error;
3. for now, the returned value from an async function is ignored (same as next(null, 'foo'));

Later:

1. we discuss if the returned value of an async function (and maybe `next(null, 'foo')) is something we want to support;

Can we agree on that for now?

cc/ @restify/current-core

[cprussin]

100% agreed with all points @ghermeto

[ghermeto]

merged