feat: replace glob-to-regexp with URLPattern

[eduardoboucas]

Which problem is this pull request solving?

Replaces the glob-to-regexp library with the web platform standard URLPattern (using a polyfill, since it's not yet part of Node.js).

This should retain the same functionality advertised in our docs while paving the way for https://github.com/netlify/pod-compute/issues/459.

[danez]

I don't think this is a good idea. This will most probably not be available once this is available in Node.js natively. Also looking at the docs of how URLPattern is supposed to be used, I do not think it matches our usecase of converting a glob to a regex.

[danez]

The mdn docs mention this library: https://github.com/pillarjs/path-to-regexp maybe we should use this?

[danez]

https://developer.mozilla.org/en-US/docs/Web/API/URL_Pattern_API

[eduardoboucas]

This will most probably not be available once this is available in Node.js natively.

I know that this property will not be exposed by the native URLPattern because it's not part of the spec, but that doesn't mean we can't continue to use the polyfill with its comprehensive test suite even after the primitive makes its way into Node natively.

Also looking at the docs of how URLPattern is supposed to be used, I do not think it matches our usecase of converting a glob to a regex.

The native URLPattern does not emit regular expressions, it just uses them internally. That's why in this PR I'm exposing the internal property where the regex is stored. The way we're using it is definitely not how it's built to be used; we're using it opportunistically to leverage its spec-backed syntax but emit a different output.

The mdn docs mention this library: https://github.com/pillarjs/path-to-regexp maybe we should use this?

That is a user-defined library though, not a standard. The whole point of this is to say that the path expressions you write when defining edge functions use the URLPattern spec from the web platform, which you can experiment with equally in Deno, Node, or the browser.

Using path-to-regexp wouldn't bring any benefits over the library we use at the moment, since they're both defined in user space.

[danez]

Okay, up to you. I personally feel this is a terrible idea to rely on the internals and implementation details of other packages.

[eduardoboucas]

I appreciate that, but can you help find a better idea?

[Skn0tt]

I share the sentiment of this feeling "bad", but I think it's an example of us knowing the rules enough that we're allowed to break them. Since we're using a polyfill, the underlying implementation won't depend on the runtime, and since we have tests we'll notice when a change in the polyfill breaks this. I think it's fine - and if we find problems, as a last resort we can still reimplement the spec ourselves.

[ascorbic]

My concern here is that we're opening ourselves to the possibility of the internals of the polyfill changing, as it's not a public API. At the very least we should pin an exact version, but personally I'd be more inclined to say we should vendor the polyfill and add the subclass to our fork.

[eduardoboucas]

At the very least we should pin an exact version

Totally. I actually meant to do this but ended up forgetting about it. Done in b308434.

My concern here is that we're opening ourselves to the possibility of the internals of the polyfill changing, as it's not a public API

If the internals change and the property we're using disappears or gets renamed, our own tests would catch it.

I'm not opposed to using our own fork if this makes people more comfortable — in fact, we do already have a fork, which I created in order to push a change upstream.

But if we go down that path, I would rather keep it on its own repo from where we can easily merge upstream changes, run the tests, etc. Not sure what we get by vendoring it in with Edge Bundler?

[Skn0tt]

Looks good. I left a point on performance, which we can address in a follow-up though.

[Skn0tt]

I've checked the impact of changing from /.* to (?:/(.*)) with this go benchmark:

import (
	"regexp"
	"testing"
)

func BenchmarkCompileGroup(b *testing.B) {
	for n := 0; n < b.N; n++ {
		regexp.Compile("^/f2(?:/(.*))/?$")
	}
}

func BenchmarkCompile(b *testing.B) {
	for n := 0; n < b.N; n++ {
		regexp.Compile("^/f2/.*/?$")
	}
}

func BenchmarkExecuteGroup(b *testing.B) {
	r, _ := regexp.Compile("^/f2(?:/(.*))/?$")
	for n := 0; n < b.N; n++ {
		r.MatchString("/f2/test/")
		r.MatchString("/different")
	}
}

func BenchmarkExecute(b *testing.B) {
	r, _ := regexp.Compile("^/f2/.*/?$")
	for n := 0; n < b.N; n++ {
		r.MatchString("/f2/test/")
		r.MatchString("/different")
	}
}

It's naïve, so i'm not sure how much we can trust it, but it shows that the group has a slight negative impact on performance (while having the same semantics, I think):

=== RUN   BenchmarkCompileGroup
BenchmarkCompileGroup
BenchmarkCompileGroup-10          461311              2570 ns/op         5000 B/op          61 allocs/op
=== RUN   BenchmarkCompile
BenchmarkCompile
BenchmarkCompile-10               542733              2152 ns/op         4240 B/op          53 allocs/op
=== RUN   BenchmarkExecuteGroup
BenchmarkExecuteGroup
BenchmarkExecuteGroup-10         7894125               152.6 ns/op          0 B/op           0 allocs/op
=== RUN   BenchmarkExecute
BenchmarkExecute
BenchmarkExecute-10              8257365               145.7 ns/op          0 B/op           0 allocs/op
PASS

This is a ~17% hit in compile time and ~5% in matching time.
Is there an easy way for us to prevent this change from adding capture groups to the output?

[eduardoboucas]

We could remove capturing groups ourselves, since we're parsing the regular expression into an AST and transforming it. But I don't think that's a huge priority.

[Skn0tt]

Okay 👍 Maybe we can capture this in an issue, so we don't forget about it.

[Skn0tt]

Put the change behind a flag, just to be safe. I'll deploy + start rolling this out tomorrow.

[eduardoboucas]

Put the change behind a flag, just to be safe. I'll deploy + start rolling this out tomorrow.

LGTM!