src: implement whatwg's URLPattern spec #56452

anonrig · 2025-01-03T16:07:05Z

Work in progress. Opening to receive some early response. Not ready to land.

Co-authored-by: Daniel Lemire (@lemire)

Blocked

This is blocked from landing due to the old macOS machines we use in our infrastructure (cc @nodejs/build)

Notes:

Ada now requires C++20
URLPattern is now a global class.
URLPattern is also exposed in node:url module
Ada now enables exceptions just like V8. This is done because std::regex, the default regex library of C++ does not have any non-exception API surface like std::filesystem. The alternative to not enabling exceptions is to bundle Ada with a regex library or implementing it's own regex parser, which is too much work for URLPattern at this stage. Further Ada releases can support such changes to disable exceptions.

TODOs

Pass all web-platform tests
Release Ada v3 before landing this PR
Make sure to split all changes to multiple commits
Add @lemire as co-author to all commits
Land upstream pull-request implement URLPattern ada-url/ada#785
Add documentation for global and node:url module declarations.

cc @nodejs/cpp-reviewers

Fixes #40844

nodejs-github-bot · 2025-01-03T16:07:11Z

anonrig · 2025-01-03T16:12:16Z

test/parallel/test-shadow-realm-globals.js

@@ -24,4 +24,4 @@ for (const item of items) {
  leaks.push(item);
 }

-assert.deepStrictEqual(leaks, []);
+assert.deepStrictEqual(leaks, ['URLPattern']);


@legendecas Is this correct? How can I make sure it is not exposed?

The interfaces exposed in lib/internal/bootstrap/web/exposed-wildcard.js should be annotated in the WebIDL as [Exposed=*].

https://urlpattern.spec.whatwg.org/#urlpattern-class defines that URLPattern is annotated as [Exposed=(Window,Worker)], so it should be exposed in lib/internal/bootstrap/web/exposed-window-or-worker.js.

This reminds why not exposing URLPattern as [Exposed=*]. whatwg/urlpattern#236 is the pending work to do it.

targos · 2025-01-03T16:21:56Z

Ada now enables exceptions just like UV and V8

Can you elaborate? libuv is a C library so I don't think exceptions exist there, and I'm pretty sure V8 is built with exceptions disabled.

anonrig · 2025-01-03T16:23:43Z

Ada now enables exceptions just like UV and V8

Can you elaborate? libuv is a C library so I don't think exceptions exist there, and I'm pretty sure V8 is built with exceptions disabled.

My bad UV does not enable exceptions. Referencing v8.gyp file:

{
  'target_name': 'torque_base',
  'type': 'static_library',
  'toolsets': ['host', 'target'],
  'sources': [
    '<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn"  "\\"torque_base.*?sources = ")',
  ],
  'dependencies': [
    'v8_shared_internal_headers',
    'v8_libbase',
  ],
  'defines!': [
    '_HAS_EXCEPTIONS=0',
    'BUILDING_V8_SHARED=1',
  ],
  'cflags_cc!': ['-fno-exceptions'],
  'cflags_cc': ['-fexceptions'],
  'xcode_settings': {
    'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',  # -fexceptions
  },
  'msvs_settings': {
    'VCCLCompilerTool': {
      'RuntimeTypeInfo': 'true',
      'ExceptionHandling': 1,
    },
  },
}

targos · 2025-01-03T16:25:11Z

This is not really V8. It's a build-time executable (torque) used to generate code for V8

src/node_url_pattern.cc

jasnell · 2025-01-03T16:44:56Z

src/node_url_pattern.cc

+  ada::url_pattern_options options{};
+  Local<Value> ignore_case;
+  if (obj->Get(env->context(),
+               FIXED_ONE_BYTE_STRING(env->isolate(), "ignoreCase"))


consider adding to env-properties.h

jasnell · 2025-01-03T16:45:59Z

src/node_url_pattern.cc

+
+MaybeLocal<Value> URLPattern::Hash() const {
+  auto context = env()->context();
+  return ToV8Value(context, url_pattern_.get_hash());


I think the key challenge here is that this will copy the string on every call. Any chance of memoizing the string once created.

jasnell · 2025-01-03T16:50:01Z

src/node_url_pattern.cc

+URLPattern::URLPattern(Environment* env,
+                       Local<Object> object,
+                       ada::url_pattern&& url_pattern)
+    : BaseObject(env, object) {


We likely should introduce this as experimental in the first release, even if it graduates from experimental quickly. There should likely be a warning emitted on the first construction.

jasnell · 2025-01-03T16:50:35Z

lib/internal/url.js

@@ -1571,6 +1572,7 @@ module.exports = {
  toPathIfFileURL,
  installObjectURLMethods,
  URL,
+  URLPattern,


Needs docs added.

jasnell · 2025-01-03T16:53:50Z

src/node_url_pattern.cc

+  if (!url_pattern->Protocol().ToLocal(&result)) {
+    return;
+  }
+  info.GetReturnValue().Set(result);


Suggested change

if (!url_pattern->Protocol().ToLocal(&result)) {

return;

}

info.GetReturnValue().Set(result);

if (url_pattern->Protocol().ToLocal(&result)) {

info.GetReturnValue().Set(result);

}

a bit more compact to invert the checks on these.

src/node_url_pattern.cc

jasnell · 2025-01-03T17:02:36Z

src/node_url_pattern.cc

+void URLPattern::New(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  CHECK(args.IsConstructCall());


since this constructor is exposed directly to users, this should throw an exception rather than abort

jasnell · 2025-01-03T17:05:16Z

Can you also include a fairly simple benchmark?

codecov · 2025-01-03T17:33:08Z

Codecov Report

Attention: Patch coverage is 80.99174% with 69 lines in your changes missing coverage. Please review.

Project coverage is 88.01%. Comparing base (ca69d0a) to head (af23313).
Report is 62 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_url_pattern.cc	81.07%	31 Missing and 36 partials ⚠️
src/node_url_pattern.h	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56452      +/-   ##
==========================================
- Coverage   88.54%   88.01%   -0.54%     
==========================================
  Files         657      662       +5     
  Lines      190393   191249     +856     
  Branches    36552    36404     -148     
==========================================
- Hits       168582   168326     -256     
- Misses      14998    16031    +1033     
- Partials     6813     6892      +79

Files with missing lines	Coverage Δ
lib/internal/bootstrap/web/exposed-wildcard.js	`99.14% <100.00%> (+0.01%)`	⬆️
lib/internal/url.js	`95.79% <100.00%> (-1.88%)`	⬇️
lib/url.js	`98.94% <100.00%> (-1.06%)`	⬇️
src/node_binding.cc	`83.66% <ø> (ø)`
src/node_external_reference.h	`100.00% <ø> (ø)`
src/node_url_pattern.h	`0.00% <0.00%> (ø)`
src/node_url_pattern.cc	`81.07% <81.07%> (ø)`

... and 135 files with indirect coverage changes

lukeed · 2025-01-03T17:45:35Z

src/node_url_pattern.cc

+    BufferValue input_buffer(env->isolate(), args[0]);
+    CHECK_NOT_NULL(*input_buffer);
+    input = input_buffer.ToString();
+  } else if (args[0]->IsObject()) {


It’s not part of spec but can we add a “is URL instance” fast path here? To avoid re-normalizing each URL property (as with the JsObject route)

Yes, we can. I haven't started working on optimizations yet due to the inconsistencies between the spec and WPT.

mcollina

I don’t think this is a good pattern to land in Node.js. Specifically, a server using this will create one per route and iterate in a loop. This will be slow, specifically if you need to match the last of the list.

(This feedback was provided when URLPattern was standardized and essentially ignored).

For this to be useful, we would need to have a Node.js-specific API to organize these URLPattern in a radix prefix trie and actually do the matching all at once.

I can possibly be persuaded that we need this for Web platform compatibility, but it’s not that popular either (unlike fetch()).

mcollina · 2025-01-03T19:01:42Z

@jasnell I’ll try to build this and get a benchmark going against the ecosystem routers.

anonrig · 2025-01-03T19:03:47Z

@jasnell I’ll try to build this and get a benchmark going against the ecosystem routers.

Right now, this pull-request does not pass WPT, and not at all optimized. Any benchmarks will not be beneficial.

jasnell · 2025-01-03T19:07:27Z

I can possibly be persuaded that we need this for Web platform compatibility, but it’s not that popular either (unlike fetch()).

It's not a great spec but it is implemented in Deno, ~~Bun~~, Workers, and browsers. We're looking at adding it to the WinterTC Common Minimum set. Node.js' lack of support is really the only thing holding that up right now. While I recognize that you don't like this particular spec, I think compat and consistency with other runtimes is a worthwhile goal here. We should land this then keep following up with optimizations and improvements. It certainly is not the first web platform api that is suboptimal from a performance pov

jasnell · 2025-01-03T19:59:56Z

@nodejs/tsc ... please review and weigh in. There is definite demand for this API as we'e had a number of folks ask over the years. It's also implemented in other runtimes and is a web platform API standard. On the downside it's not the best pattern or most performant approach. Ideally we'd be able to resolve without referring to a vote. To be certain, we know this approach isn't the most performant so how it performs relative to other router impls should not be part of the consideration here.

marco-ippolito · 2025-01-03T21:06:30Z

I'm +1 on adding it to Node.js, I like more WP compatibility and I feel like it's an api that could be useful for libraries.
I will not comment on the implementation details since I'm not an expert on the subject

ronag

We have landed plenty of inefficient Web API's. Don't see why this would be different. I'm of the consistent opinion that we should implement them but strongly discourage their use for any performance sensitive code.

mon-jai · 2025-01-04T00:55:44Z

Just out of curiosity, why can't we port the Chromium's implementation but instead need to reimplement this ourselves? (I am not a maintainer)

jasnell · 2025-01-04T17:04:14Z

Just out of curiosity, why can't we port the Chromium's implementation but instead need to reimplement this ...

The chromium implementation depends on quite a few chromium/blink internals that we don't have here. That kind of reuse is difficult to just make happen. This ada-based implementation is standalone, has no other dependencies, etc. It's actually easier overall just to write it from scratch than to repurpose that other impl.

typings/internalBinding/url_pattern.d.ts

domenic · 2025-01-06T04:39:53Z

One worry worth highlighting here is that, IIUC, the architecture of this PR will use a separate regexp library for the regexps that show up in the URLPattern constructor, vs. the ones that show up in the RegExp constructor.

That could be confusing for developers, as I assume the feature overlap will not be the same. For example, I wonder what results you would get after this PR for:

new URLPattern("https://example.com/:id([\\p{Decimal_Number}--[1-9]])").test("https://example.com/0");

new RegExp("[\\p{Decimal_Number}--[1-9]]", "v").test("0");

In Chromium we have architected our URLPattern implementation into two parts: one, liburlpattern, which is meant to have very few dependencies, and largely works to produce regexp pattern strings. The second part, in blink/renderer/core/url_pattern, implements more parts of the spec, including bridging to V8's regexp engine.

I suspect a similar architecture would not fit that well for you, given the different boundaries between Ada / Node.js. But maybe Ada could have some sort of regexp-matcher-callback system which would allow the use of V8's regexps instead of std::regex.

anonrig requested review from jasnell and RafaelGSS January 3, 2025 16:07

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 3, 2025

anonrig commented Jan 3, 2025

View reviewed changes

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 3, 2025

anonrig added macos Issues and PRs related to the macOS platform / OSX. blocked PRs that are blocked by other issues or PRs. build-agenda labels Jan 3, 2025

anonrig requested a review from Qard January 3, 2025 16:27