Expose router engine for other implementations

[blakeembrey]

There's still a couple of things to decide, but it works wonderfully for what I need and can plug any other implementation in that supports the same function return syntax. Only issue I see is debugging since it currently relies heavily on human-readable paths to be passed to the engine.

Closes #12

[blakeembrey]

@dougwilson Thoughts on an implementation like this?

[dougwilson]

Hey, sorry, I've just been busy and haven't been able to look over this changeset yet (it's really big). The only thing I've noticed so far is you deleted a few tests, signaling it's not backwards-compatible, but I haven't look at anything else just yet.

[blakeembrey]

@dougwilson It should be backward compatible, those tests seemed to be testing implementation details by using route.path but that's the same note as the debugging issue I mentioned. Since it's using a function now, testing for route.path doesn't make so much sense and neither does logging it. So maybe we add a property to functions that represent route.path that can be logged?

Edit: We can easily keep Router.Route expose too to fix that change.

[dougwilson]

Gotcha. So after the change, what does req.route become? It people using it now cannot drop in this change set, then that's by definition not backwards-compatible :)

[dougwilson]

For example, there seem to be many, many people using req.route.path, but I guess that's missing now? Since it was a publicly-exposed thing, if it's missing and all they did was upgrade this module, it would have to be under a major version bump is all. I'm not saying we cannot, I'm just saying what I saw so far :)

[blakeembrey]

The only change is that req.route.path no longer exists. If that's an issue with breaking backward compatibility then we could keep passing the path through easily. It'd fix the debugging issues too, to keep passing that around.

[blakeembrey]

Ok, good to know. I'll add that functionality back now and ping you again 👍

[blakeembrey]

By the way, one thing I noticed was strict: false on .use instead of using the this.strict option - is that intentional?

[blakeembrey]

@dougwilson No backward incompatibility now.

Edit: Added error throwing for missing match function and/or missing path argument in engine.

[blakeembrey]

@dougwilson Sorry to be so pushy, I just really want to publish a bunch of middleware on top of this 😄 Do you have any thoughts on the implementation?

[dougwilson]

Sure. Sorry, it's hard to look through " 801 additions and 739 deletions" :) Do you have any documentation of how this new feature would be used? (so I can at least maybe only review the external API :P).

[dougwilson]

I also don't completely understand what you're doing with this truth() function in a few of the files.

[blakeembrey]

The implementation layer is pretty much index.js and most of the changes are just moving index.js to engine.js and making the layers use a match function instead of regexp. It's not actually a lot of changes, but the files moving made it a bit messy (sorry!)

I can also publish the module implementation I've implemented to show you how I'm using it.

[blakeembrey]

I also don't completely understand what you're doing with this truth() function in a few of the files.

Yeah, sorry. Maybe there's a better way to document it, but it's basically the "fast match" route from before that you always expected to match (E.g. use). It's a function that always evaluates to true, but I'll think if there's a better implementation. Thoughts?

[dougwilson]

I can also publish the module implementation I've implemented to show you how I'm using it.

That's fine, and we can also exchange it privately if you feel that necessary :) But we'll probably want docs in the readme eventually, haha.

[blakeembrey]

I can try my hand at some simple documentation at the bottom of the readme.

[dougwilson]

but I'll think if there's a better implementation. Thoughts?

I don't know enough about the implementation to really comment :)

[dougwilson]

From the title:

Expose router engine for other implementations

Based on your file names, I would expect that you'd have an exports of Engine? I don't see that, though?

[blakeembrey]

@dougwilson Ok, done. I copied some tests over, but the bulk of the changes I just made were because I realised there were some simple optimisations to do. Moved fast_slash and decode_param to be engine implementations, not router implementations and also realised that every router implementation doesn't need to define all the methods, just needs to override Engine#route.

[blakeembrey]

Actually, I shouldn't be coding at 1am. The reason people needed to implement these specifically was because other implementations can use a different number of arguments. However, the only difference for those users now will be that they can't rely on the methods already set up and can't use the sanitizeUse utility I added. I guess the question now is, is it better to be explicit or implicit for implementors? We can always add a simple note to the readme saying that if the implementor accepts a different number of "path" arguments, they'll need to override the method implementations themselves and we can expose that as an array to them. Thoughts? After that, this is finally done 😄

[dougwilson]

Nice! And yes, that is my main concern: that the changes fit into a case where people can use it to build upon :)

[blakeembrey]

@dougwilson Well, if it helps, we've been using it in https://www.npmjs.com/package/osprey-router for the past few months. All the methods can (currently) be overridden if you need to support for arguments and I'll make that note in the README tomorrow. But maybe I'll remove the method implementations by default and add it back to the example, instead exposing the array from methods for implementors. Was there anything else you were thinking about?

Edit: Well, that was conflicting. Still can't make up my mind if the default methods loop implementation should be there or not. There's no functionality change in any case. I think if we foresee more implementation sticking with one path argument it makes sense to leave it there and be overridden by the outliers.

[blakeembrey]

@dougwilson Ok, all done and dusted. The sample implementation in the README should be adequate for anyone else to get started.

[blakeembrey]

@dougwilson Have you had any more time to review this at all? Would love to re-open the discussion around unicode URLs once this is merged.

[blakeembrey]

@dougwilson Any chance you could review this before too many more changes continue happening 😝

[blakeembrey]

@dougwilson 🙏

[dougwilson]

Sorry, I have just been so busy herding all these modules around :) I just finished looking through a lot of the Express 4.x issues, so this is on my backlog after getting 4.13.0 out to get this up to snuff and then into the 5.x branch of Express very soon to get a 5.0 beta out! I consider this issue a blocker to 5.0 beta and we really need to get 5.0 out next month, so I hope that signals the importance/timeline of this issue :)

[blakeembrey]

@dougwilson Just rebased back against the latest master release. Still looking forward to merge 😜

I just realised (maybe I did before) but this module still uses [email protected]. Shouldn't it be using 1.x now? Or will that be updated before the 5.0 release with this?

[blakeembrey]

@dougwilson Rebased against 1.1.3. Anything I can do to speed this along? By the way, the best way to diff this is to compare the new engine.js against the old index.js. That should make diffing it easier, since things are pretty much the same minus a couple of parts to decouple the engine.

[blakeembrey]

@dougwilson I need to continue working on this branch for promise support and router exiting, so I'm going to close this and reopen under a new PR.