use ada-url's url_pattern implementation #3529

anonrig · 2025-02-11T22:21:33Z

Since we moved URLPattern implementation to Ada and shared it with Node.js, we can now use it as a dependency and get rid of our implementation. This will allow us to have higher compatibility with the web-platform tests and spec, and reduce our codebase by 3800 lines...

Additionally with the help of @npaun, we now run web-platform tests and can get rid of our custom URLPattern tests for better web compatibility and maintenance, since WPT is always changing!

github-actions · 2025-02-11T22:36:28Z

The generated output of @cloudflare/workers-types has been changed by this PR. If this is intentional, run bazel build //types && rm -rf types/generated-snapshot && cp -r bazel-bin/types/definitions types/generated-snapshot to update the snapshot. Alternatively, you can download the full generated types: https://github.com/cloudflare/workerd/actions/runs/13399650587/artifacts/2612076737

Full Type Diff

diff -r bazel-bin/types/definitions/2021-11-03/index.d.ts types/generated-snapshot/2021-11-03/index.d.ts
2639d2638
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2021-11-03/index.ts types/generated-snapshot/2021-11-03/index.ts
2651d2650
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-01-31/index.d.ts types/generated-snapshot/2022-01-31/index.d.ts
2665d2664
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-01-31/index.ts types/generated-snapshot/2022-01-31/index.ts
2677d2676
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-03-21/index.d.ts types/generated-snapshot/2022-03-21/index.d.ts
2683d2682
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-03-21/index.ts types/generated-snapshot/2022-03-21/index.ts
2695d2694
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-08-04/index.d.ts types/generated-snapshot/2022-08-04/index.d.ts
2684d2683
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-08-04/index.ts types/generated-snapshot/2022-08-04/index.ts
2696d2695
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-10-31/index.d.ts types/generated-snapshot/2022-10-31/index.d.ts
2687d2686
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-10-31/index.ts types/generated-snapshot/2022-10-31/index.ts
2699d2698
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-11-30/index.d.ts types/generated-snapshot/2022-11-30/index.d.ts
2692d2691
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2022-11-30/index.ts types/generated-snapshot/2022-11-30/index.ts
2704d2703
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2023-03-01/index.d.ts types/generated-snapshot/2023-03-01/index.d.ts
2694d2693
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2023-03-01/index.ts types/generated-snapshot/2023-03-01/index.ts
2706d2705
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2023-07-01/index.d.ts types/generated-snapshot/2023-07-01/index.d.ts
2694d2693
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/2023-07-01/index.ts types/generated-snapshot/2023-07-01/index.ts
2706d2705
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/experimental/index.d.ts types/generated-snapshot/experimental/index.d.ts
2766d2765
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/experimental/index.ts types/generated-snapshot/experimental/index.ts
2778d2777
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/oldest/index.d.ts types/generated-snapshot/oldest/index.d.ts
2639d2638
<   get hasRegExpGroups(): boolean;
diff -r bazel-bin/types/definitions/oldest/index.ts types/generated-snapshot/oldest/index.ts
2651d2650
<   get hasRegExpGroups(): boolean;

src/workerd/api/urlpattern.c++

src/workerd/io/BUILD.bazel

anonrig · 2025-02-18T20:39:20Z

Other than putting this behind a flag like @jasnell recommended, this is ready to review.

fhanau · 2025-02-18T20:56:02Z

src/workerd/api/wpt/urlpattern-test.ts

    expectedFailures: [
-      // Each of these *ought* to pass. They are included here because we


I assume that this comment is no longer needed since the tests themselves are invalid here.

fhanau · 2025-02-18T20:58:01Z

src/workerd/api/urlpattern.h

@@ -4,8 +4,12 @@

 #pragma once

+#include "ada.h"


As per KJ style, this should be <ada.h> (some people on the team have strong opinions about this).

fhanau · 2025-02-18T21:03:05Z

src/workerd/api/BUILD.bazel

@@ -249,6 +249,7 @@ wd_cc_library(
    kj_test(
        src = f,
        deps = [
+            ":urlpattern",


There is no such target; it should be worthwhile to create one by isolating it from io as noted in the TODO at the top of this file. It is not necessary to do so in this PR though as doing so may require downstream changes. Furthermore, urlpattern.h is not used by any of these tests (it is needed for api-rtti-test so it would be correct to add it there).

I created urlpattern target on a different PR

It exists: #3546

fhanau · 2025-02-18T21:04:28Z

justfile

@@ -21,7 +21,7 @@ build *args="//...":
  bazel build {{args}}

 build-asan *args="//...":
-  just build {{args}} --config=asan --sandbox_debug


Changes to this file are already in #3551, let's avoid duplicating them here

npaun · 2025-02-18T23:05:53Z

src/workerd/api/urlpattern.h

-#include <workerd/jsg/url.h>
+
+#include <optional>
+#include <string_view>


Suggested change

#include <string_view>

#include <kj/array.h>

#include <kj/one-of.h>

#include <kj/string.h>

#include <string>

#include <string_view>

#include <utility> // For std::move

#include <vector>

For include-what-you-use reasons.

npaun · 2025-02-18T23:23:16Z

src/workerd/api/urlpattern.c++

+  ada::url_pattern_options options{};
+  options.ignore_case = ignoreCase.orDefault(false);
+  return options;


Suggested change

ada::url_pattern_options options{};

options.ignore_case = ignoreCase.orDefault(false);

return options;

return {.ignore_case = ignoreCase.orDefault(false)};

erikcorry · 2025-02-19T10:23:00Z

src/workerd/api/urlpattern.c++

+  jsg::Lock& js = jsg::Lock::from(v8::Isolate::GetCurrent());
+  KJ_IF_SOME(matches, pattern.getHandle(js)(js, kj::StringPtr(input.data(), input.size()))) {
+    std::vector<std::optional<std::string>> results;
+    results.reserve(matches.size());


I think we are reserving one more slot than we need here?

Yes but adding -1 adds a branch (checking if size is bigger than 0), which reduces the impact of this optimization

erikcorry · 2025-02-19T10:27:12Z

src/workerd/api/urlpattern.c++

 }

+#define V(uppercase, lowercase)                                                                    \


Code like this is clearer if you give V a meaningful name like DEFINE_GETTER.