Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Api: URL Pattern API implementation #136

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Conversation

erivas-ligo
Copy link
Contributor

@erivas-ligo erivas-ligo commented Nov 2, 2023

Description

Related issue: 🚧 API: URLPattern #100

Dependencies: urlpattern

This PR uses urlpattern crate (which was created by Deno developers) to implement URLPattern.

The underlying crate, and thus URLPattern as defined here (and also Deno) has difference with the URL Pattern specified: it does not support the ignoreCase option. There is an open PR in urlpattern to support it, but it is already quite old, and not sure if it will be supported.

A single NativeClass is introduced (for URLPattern). For the other components (URLPatternInit, URLPatternInput, URLPatternComponentResult, URLPatternResult) this PR does not introduce native classes, and instead uses needed TryFromJs and IntoJs implementations.

Manual testing

nix develop
cargo run --bin jstz -- repl
>> (function () {
  const pattern = new URLPattern("https://deno.land/foo/:bar");
  console.log(pattern.protocol == "https");
  console.log(pattern.protocol == "https");
  console.log(pattern.hostname == "deno.land");
  console.log(pattern.pathname == "/foo/:bar");
  console.log(pattern.test("https://deno.land/foo/x"));
  console.log(!pattern.test("https://deno.com/foo/x"));
  match = pattern.exec("https://deno.land/foo/x");
  console.log(match);
  console.log(match.pathname.input == "/foo/x");
  // false, but also false in Deno/Chrome
  console.log(match.pathname.groups == { bar: "x" });
})();
[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] [object Object]
[🪵] true
[🪵] false
>> (function () {
  const pattern = new URLPattern("/foo/:bar", "https://deno.land");
  console.log(pattern.protocol == "https");
  console.log(pattern.hostname == "deno.land");
  console.log(pattern.pathname == "/foo/:bar");
  console.log(pattern.test("https://deno.land/foo/x"));
  console.log(!pattern.test("https://deno.com/foo/x"));
  const match = pattern.exec("https://deno.land/foo/x");
  console.log(match);
  console.log(match.pathname.input == "/foo/x");
  // false, but also false in Deno/Chrome
  console.log(match.pathname.groups == { bar: "x" });
})();
[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] [object Object]
[🪵] true
[🪵] false
>> (function () {
  const pattern = new URLPattern({
    pathname: "/foo/:bar",
  });
  console.log(pattern.protocol == "*");
  console.log(pattern.hostname == "*");
  console.log(pattern.pathname == "/foo/:bar");
  console.log(pattern.test("https://deno.land/foo/x"));
  console.log(pattern.test("https://deno.com/foo/x"));
  console.log(!pattern.test("https://deno.com/bar/x"));
  console.log(pattern.test({ pathname: "/foo/x" }));
})();

[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] true
[🪵] true
>>

Testing

Checklist

  • Changes follow the existing code style (use make fmt-check to check)
  • Tests for changes have been added
  • Internal documentation has been added (if appropriate)
  • Testing instructions have been added to PR

@erivas-ligo erivas-ligo self-assigned this Nov 2, 2023
@erivas-ligo erivas-ligo linked an issue Nov 2, 2023 that may be closed by this pull request
Copy link
Collaborator

@johnyob johnyob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉

Some minor nits to fix

jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
jstz_api/src/urlpattern/mod.rs Outdated Show resolved Hide resolved
@erivas-ligo
Copy link
Contributor Author

Thanks for the review! Tried to address all of the comments.

@johnyob
Copy link
Collaborator

johnyob commented Nov 9, 2023

LGTM! 🎉

I made a review commit to:

  • Fix handling of baseURL (incorrect property name in TryFromJs and missing in IntoJS)
  • Added some TODO comments on handling errors in IntoJs (we should switch to a trait called TryIntoJs soon™️)
  • Minor idiomatic changes to Rust code (namely preferring field access over destructuring)

This PR is ready to be merged, but I'd prefer to hold off on merging it until our testing infrastructure is done.

Also before merging please tidy up the git history (either by squashing or rebasing and rewriting history)

@Laucans Laucans removed a link to an issue Nov 10, 2023
This was linked to issues Nov 10, 2023
@erivas-ligo erivas-ligo force-pushed the erivas@api/url_pattern branch 2 times, most recently from 4a5af61 to 71f9f44 Compare November 23, 2023 15:46
@johnyob
Copy link
Collaborator

johnyob commented Nov 23, 2023

@erivas-ligo are wpt tests enabled? If not, then this PR will be stuck until so

@erivas-ligo erivas-ligo marked this pull request as ready for review November 24, 2023 17:44
@erivas-ligo erivas-ligo merged commit 86e4c1a into main Nov 24, 2023
5 checks passed
@erivas-ligo erivas-ligo deleted the erivas@api/url_pattern branch November 24, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants