next is missing in the operation wrapper

[tfogo]

Ran into an issue while implementing passport with carbon. Perhaps this is deliberate but here's what I found:

Expected Behaviour

Endpoint functions should have the signature function(req, res, next) so that next can be invoked.

__(() => {
  module.exports = o({
    _type: carbon.carbond.Service,
    port: 8888,
    endpoints: {
      'user/:id': o({
        _type: carbon.carbond.Endpoint,
        get: (req, res, next) => {
          next();
        }
      })
    }
  })
});

Actual Behaviour

Endpoint functions have the signature function(req, res). next is missing from Operation.service's arguments at Endpoint.js lines 98:99 and from the function call at Service.js line 1483.

Here's an example you can actually test this with:

var carbon = require('carbon-io');
var o = carbon.atom.o(module).main;
var __ = carbon.fibers.__(module);

__(() => {
  module.exports = o({
    _type: carbon.carbond.Service,
    port: 8888,
    endpoints: {
      'user/:id?': o({
        _type: carbon.carbond.Endpoint,
        get: (req, res, next) => {
          console.log('user/:id?');
          if (req.params.id) {
            next();
          } else {
            // something else
          }
        }
      }),
      'user/:id': o({
        _type: carbon.carbond.Endpoint,
        get: (req, res, next) => {
          console.log('user/:id');
          res.send(req.params.id);
        }
      })
    }
  })
});

Going to /user/123 should first hit the /user/:id? endpoint then be passed to the /user/:id endpoint. Instead, you get a Typer error: next is not a function.

[tfogo]

Just gonna submit a PR on this since I have the changes ready on my fork, let me know if I haven't understood it correctly.

[willshulman]

We had not considered supporting this. Are there other examples of where calling next() has meaning in the context of an Endpoint? (examples other than :id? vs :id)

[willshulman]

Another question: what was the existing behavior when a client hits /user/123? Was it hitting the user/:id? endpoint or the user/:id endpoint?

[tfogo]

Re existing behaviour, it was hitting user/:id? first. Routes are added to the middleware stack in the order in which they are defined and express will use the first matching route.

For other examples, Passport uses it for error handling. Not having next makes some types errors not particularly graceful. It's also conceivable people might want to run middleware after a route (for logging or some kind of callback). But that seems like bad practice anyway.

Passport's kind of an edge case so I don't know any reasons it's definitely necessary to have it.

[willshulman]

Got it.

I was actually not aware of the question mark '?'.

I hesitate to add this to carbond because the idea with the endpoint definitions is that incoming paths only match one endpoint. In fact, we might consider restricting the grammar here (we don't advertise express as the underlying impl so people should not expect it to be exactly the same).

Introducing the idea that more than one can match, and that you can iterate through the matches using next() would be a new idea and in my opinion possibly confusing, and I don't think we miss much by not supporting it.

Also, carbond middleware all come before the endpoint tree.

If we do not do this will you have issues using passport? Is there a workaround for you to use passport without its errors getting swallowed (if I understand the problem correctly)?

[tfogo]

Express's route paths have some basic regex too!

The ? isn't usually used with next() - I just made an example using it. Generally I've just seen ? used for optional params that are handled without use of next. /dates/:start-:end? could be an example.

So the issues of restricting the use of next and restricting the grammar of Express routes are separate. I imagine people would expect routes to be Express-like. Why would we want to actively restrict the routes people could use? What about keeping the feature but only documenting what we think is best practice?

Re next, Passport works fine but potentially swallowing errors is not ideal. Adding next would be the easiest way to make sure errors aren't swallowed. As an aside, I think it would be interesting to build an authenticator that integrates with passport - but we'd need to use carbon.carbond.Endpoint and that needs to use next for errors not to be swallowed. I guess the question here is again, why not keep the feature just in case but only document the best practice?

[willshulman]

Thanks. I will think about it and we can discuss live when we get a chance. I think my main source of caution with full regex support is that it creates the situation where more than one endpoint might match a given request path.

When you say we should allow it and document best practice do you mean to say we should allow full regex support but document the pitfall of defining more than one endpoint that could match the same path?

We actually have this issue without full regexes currently in that you could have foo/:id and foo/:eyedee which will both match /foo/bar but we were planning to fail on startup in this case to prevent the customer from making the mistake.

[tfogo]

Yeah I think it would be good to talk more about the potential pitfalls in person so I fully understand what they are.