src: implement whatwg's URLPattern spec #56452

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

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

Notes:

Ada now requires C++20
URLPattern is exposed through node:url module

TODOs

Removed exception flag requirement
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

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

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: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.

I'll follow up on a different PR for this

src/node_url_pattern.cc

lib/internal/url.js

src/node_url_pattern.cc

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 78.64769% with 120 lines in your changes missing coverage. Please review.

Project coverage is 89.18%. Comparing base (f2001e3) to head (49d1fd7).
Report is 46 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_url_pattern.cc	78.77%	58 Missing and 60 partials ⚠️
src/node_url_pattern.h	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56452      +/-   ##
==========================================
- Coverage   89.21%   89.18%   -0.03%     
==========================================
  Files         663      665       +2     
  Lines      192001   192574     +573     
  Branches    36921    37046     +125     
==========================================
+ Hits       171286   171749     +463     
- Misses      13582    13635      +53     
- Partials     7133     7190      +57

Files with missing lines	Coverage Δ
lib/internal/url.js	`97.55% <100.00%> (-0.13%)`	⬇️
lib/url.js	`100.00% <100.00%> (ø)`
src/node_binding.cc	`83.66% <ø> (ø)`
src/node_errors.h	`85.00% <ø> (ø)`
src/node_external_reference.h	`100.00% <ø> (ø)`
src/node_url_pattern.h	`0.00% <0.00%> (ø)`
src/node_url_pattern.cc	`78.77% <78.77%> (ø)`

... and 31 files with indirect coverage changes

src/node_url_pattern.cc

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.

ljharb

I can't review the C++ but the parts I can look fine

ljharb · 2025-01-31T06:00:21Z

doc/api/url.md

+
+Input can be a string or an object providing the individual URL parts. The
+object members can be any of `protocol`, `username`, `password`, `hostname`,
+`port`, `pathname`, `search`, `hash` or `baseURL`.


Suggested change

`port`, `pathname`, `search`, `hash` or `baseURL`.

`port`, `pathname`, `search`, `hash`, or `baseURL`.

ljharb · 2025-01-31T06:00:36Z

doc/api/url.md

+
+Input can be a string or an object providing the individual URL parts. The
+object members can be any of `protocol`, `username`, `password`, `hostname`,
+`port`, `pathname`, `search`, `hash` or `baseURL`.


Suggested change

`port`, `pathname`, `search`, `hash` or `baseURL`.

`port`, `pathname`, `search`, `hash`, or `baseURL`.

ljharb · 2025-01-31T06:02:16Z

typings/internalBinding/url_pattern.d.ts

+  constructor(input: Record<string, string> | string, options?: { ignoreCase: boolean });
+  constructor(input: Record<string, string> | string, baseUrl?: string, options?: { ignoreCase: boolean });


Suggested change

constructor(input: Record<string, string> | string, options?: { ignoreCase: boolean });

constructor(input: Record<string, string> | string, baseUrl?: string, options?: { ignoreCase: boolean });

constructor(input: Record<string, string> | string, options?: { ignoreCase?: boolean });

constructor(input: Record<string, string> | string, baseUrl?: string, options?: { ignoreCase?: boolean });

since an empty options object is still permitted

anonrig · 2025-01-31T17:45:11Z

@nodejs/tsc @nodejs/platform-smartos ... I can not justify the reasoning for waiting for smartOS build to succeed for more than 5.5 hours (https://ci.nodejs.org/job/node-test-commit-smartos/58956/nodes=smartos22-x64/) but here we are... The latest discussion didn't involve cases like this, we only had a discussion about flaky tests.

Please ask yourselves, if you were a new contributor, would you still continue contributing to Node.js after waiting for 5 hours for CI to succeed?

bahamat · 2025-01-31T17:49:30Z

@anonrig As you know, this is currently being discussed in nodejs/build#4011 (which you opened), and there are proposals there to correct the issue. I'd like to keep the main discussion in that issue so that the conversation is all in one place.

Thanks!

anonrig · 2025-01-31T17:51:32Z

@anonrig As you know, this is currently being discussed in nodejs/build#4011 (which you opened), and there are proposals there to correct the issue. I'd like to keep the main discussion in that issue so that the conversation is all in one place.

Thanks!

@bahamat That discussion doesn't solve the frustration of this PR right now. Any suggestions on how to land this pull-request now?

nodejs-github-bot · 2025-01-31T21:36:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64891/

nodejs-github-bot · 2025-02-01T01:16:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64892/

nodejs-github-bot · 2025-02-01T02:14:03Z

Landed in 44d2648...b78df6c

Co-authored-by: Daniel Lemire <[email protected]> PR-URL: #56452 Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

ShogunPanda

Not really qualified to review C++.
The rest LGTM. Nice job buddy!

Co-authored-by: Daniel Lemire <[email protected]> PR-URL: #56452 Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Notable changes: crypto: * (SEMVER-MINOR) support --use-system-ca on Windows (Joyee Cheung) #56833 * (SEMVER-MINOR) added support for reading certificates from macOS system store (Tim Jacomb) #56599 deps: * update timezone to 2025a (Node.js GitHub Bot) #56876 * (SEMVER-MINOR) update ada to v3.0.1 (Yagiz Nizipli) #56452 deps,tools: * (SEMVER-MINOR) add zstd 1.5.6 (Jan Krems) #52100 sqlite: * (SEMVER-MINOR) allow returning `ArrayBufferView`s from user-defined functions (René) #56790 src: * set signal inspector io thread name (RafaelGSS) #56416 * set thread name for main thread and v8 worker (RafaelGSS) #56416 * set worker thread name using worker.name (RafaelGSS) #56416 * use a default thread name for inspector (RafaelGSS) #56416 src, quic: * (SEMVER-MINOR) refine more of the quic implementation (James M Snell) #56328 test: * (SEMVER-MINOR) add WPT for URLPattern (Yagiz Nizipli) #56452 url: * (SEMVER-MINOR) add URLPattern implementation (Yagiz Nizipli) #56452 zlib: * (SEMVER-MINOR) add zstd support (Jan Krems) #52100 PR-URL: TODO

Notable changes: crypto: * (SEMVER-MINOR) support --use-system-ca on Windows (Joyee Cheung) #56833 * (SEMVER-MINOR) added support for reading certificates from macOS system store (Tim Jacomb) #56599 deps: * update timezone to 2025a (Node.js GitHub Bot) #56876 * (SEMVER-MINOR) update ada to v3.0.1 (Yagiz Nizipli) #56452 deps,tools: * (SEMVER-MINOR) add zstd 1.5.6 (Jan Krems) #52100 sqlite: * (SEMVER-MINOR) allow returning `ArrayBufferView`s from user-defined functions (René) #56790 src: * set signal inspector io thread name (RafaelGSS) #56416 * set thread name for main thread and v8 worker (RafaelGSS) #56416 * set worker thread name using worker.name (RafaelGSS) #56416 * use a default thread name for inspector (RafaelGSS) #56416 src, quic: * (SEMVER-MINOR) refine more of the quic implementation (James M Snell) #56328 test: * (SEMVER-MINOR) add WPT for URLPattern (Yagiz Nizipli) #56452 url: * (SEMVER-MINOR) add URLPattern implementation (Yagiz Nizipli) #56452 zlib: * (SEMVER-MINOR) add zstd support (Jan Krems) #52100 PR-URL: #57005

Notable changes: crypto: * (SEMVER-MINOR) support --use-system-ca on Windows (Joyee Cheung) #56833 * (SEMVER-MINOR) added support for reading certificates from macOS system store (Tim Jacomb) #56599 deps: * update timezone to 2025a (Node.js GitHub Bot) #56876 sqlite: * (SEMVER-MINOR) allow returning `ArrayBufferView`s from user-defined functions (René) #56790 src: * set signal inspector io thread name (RafaelGSS) #56416 * set thread name for main thread and v8 worker (RafaelGSS) #56416 * set worker thread name using worker.name (RafaelGSS) #56416 * use a default thread name for inspector (RafaelGSS) #56416 url: * (SEMVER-MINOR) add URLPattern implementation (Yagiz Nizipli) #56452 zlib: * (SEMVER-MINOR) add zstd support (Jan Krems) #52100 PR-URL: #57005

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

test/parallel/test-shadow-realm-globals.js Outdated Show resolved Hide resolved

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