Constrained routes take 2

[airhorns]

This is about fastify/fastify#2498 (host based routing), closes #165, replaces and closes #166. This is a completed version of @AyoubElk's changes in #166 that passes the tests!

This:

• keeps support for semver based version constrained routes
• keeps the old behaviour of not matching a versioned route if no Accept-Version route is passed, which requires lots of code
• adds support for host constrained routes that only match if the Host header is equal to a string or passes a regexp test
• adds support for arbitrary constraints passed as objects that know how to inspect a request and decide if a handler should match for that request
• adds support for testing multiple constraints for a handler, and matching only handlers who pass all the constraints

Implementation details:

• find now (must) take an object of constraint values derived from the request. Before, it took an optional version, but now there are many values, so it takes an object with a version key and or others. This is a weird API if you're calling find directly, but invisible if you're calling lookup, which is what Fastify itself does. This is done to avoid extra checks and allocations for every route match. It also generates a lot of noise in this PR, sorry.
• Nodes now know how to split themselves
• Nodes no longer have a .handler and a .versionStore that store handlers separately, they have an array of handlers, and sometimes many constraint stores that know how to report which handlers should match which constraint values. The old version store is one, the host store is another, and userland code can create more.
• There's a Constrainer object that holds all the constraining strategies and that knows how to validate constraints
• Constraint testing uses fast and memory efficient bitwise operations to figure out which handlers match all the constraints (when there are constraints for a route). This is tricky but necessary because we need to return handlers that match all the constraints, not just some of them, so we need to keep some intermediate state when checking about what handlers have passed all the constraints so far.
• An optimized function to get the derivedConstraints, which is the request host and the accept-version header most of the time is new Function()'d for speed since it is run for every request match
• An optimized constraint matcher function using the bitmapping algorithm described in the comments is new Function()'d for each handler that has constraints, which creates a megamorphic callsite but only for constrainted handlers and in my testing is still faster than a monomorphic callsite with a loop

Notably, there's a huge performance regression right now, especially on static route matching. First make it work, then make it fast they say, which I have been trying but failing to do, so I'd really love any assistance or even just theories about what is causing a big slow down. Currently a Draft PR until performance is brought back in line.

Would love your feedback on the code and specifically any pointers for where I screwed up the performance! On master I see roughly 45M static route matches a second, and I see 32M on this branch, so there's a lot of work yet to do.

The remaining work:

• Aforementioned performance improvement
• CHANGELOG entry and upgrade info. The only breaking change would be to the shape of custom version strategies, which I could potentially get rid of if strictly necessary at the expense of some nasty code.

Also just FYI @AyoubElk and I have been coordinating on this off platform, I stepped in to complete the work so please direct your feedback on this feature to us here!

[mcollina]

lgtm

[airhorns]

ping @delvedor care to take another look?

[delvedor]

LGTM

Before merging this I would kindly ask you to remove the not necessary empty object you have added in some test when calling find, given that it's not required we should not have it there, otherwise we risk to create a false positive test :)

[airhorns]

LGTM

Woo!

Before merging this I would kindly ask you to remove the not necessary empty object you have added in some test when calling find, given that it's not required we should not have it there, otherwise we risk to create a false positive test :)

I think I might have been unclear about what that value is actually for because I consider that arg very much required. For the system to meet its contract, the caller needs to think about what is passed for that argument and pass something, it just might be that the value passed is undefined for performance reasons. Most direct callers of .find aren't going to be finding one hardcoded handler, it's going to be some higher level function that uses find my way to route to any of the routes registered, which means the value for the constraints argument should probably be dynamic, not just hardcoded to undefined or omitted. That's why I went through and added it to every test.

The reason is that within the realm of this library, I think constraints must be a thing that contributors care about. If they go to change the constraints implementation, or change something else that passes code to and from the bits that care about constraints, they should have to think about how the constraints are going to be passed in, and how to keep the performance of the constraint derivation high for whatever new thing they want to do. I think making it easy to ignore constraints is the wrong thing to do in this library, because (once merged) this library has committed to supporting them. I think if users of find-my-way want to not care about constraints, that's fine, they can just always pass undefined, but that's different than how we test and communicate to new contributors.

That said, I don't want to be a stick in the mud, so if it's really important to you that the tests don't get updated I can go through and revert those changes.

[delvedor]

That said, I don't want to be a stick in the mud, so if it's really important to you that the tests don't get updated I can go through and revert those changes.

Yes, it's important. If both behaviors are supported, then the test should verify that.

[airhorns]

I agree its important, and there's tests that test the behaviour with and without the parameter. If that's all thats required we're good to merge, but are you saying you want all the test cases to implicitly test the undefined value, and then have only some tests that test with the value?

[airhorns]

ping @delvedor is the final decision to remove the parameter from all the tests or can we merge?

[delvedor]

Sorry to be a PITA, but if this pr is not changing the default behavior, those tests should not be updated.

[airhorns]

Again -- it isn't that the default behaviour hasn't changed, it has. Callers of .find should and will need to think about what to pass for the third parameter. Every call to lookup will call find with a value for the third parameter. It is required in the human sense of the word, just not in the JS VM sense of the word. I get that test cleanness is a thing though -- updated to remove the changes to .find! Any other blockers?

[mcollina]

lgtm

[airhorns]

ping @delvedor anything else you'd like to see before ? Would really like to get this in!

[delvedor]

LGTM 🚀

[airhorns]

@AyoubElk would you mind giving one last review? Then I think we're ready to merge!

[AyoubElk]

LGTM

[airhorns]

Ok great -- anything left before merging?

[airhorns]

ping @delvedor can you hit the merge button? Or if there is anything outstanding let me know!

[delvedor]

@airhorns there are also others pr that needs to be merged and since this one is a breaking change I'm wondering about if we should do a minor first and then cut the major or directly go all in with the major.
What do you think @mcollina?

[mcollina]

I would just ship a major and then land the major update in FAstify.