fix(URLPattern): Handle search regex correctly in URLPattern

[yazan-abdalrahman]

[CLAassistant]

All committers have signed the CLA.

[yazan-abdalrahman]

@crowlKats

before

after

[crowlKats]

this does not seem to be complying to the specification.

however, I took a bit of a deeper look and one issue is we cannot update the testdata since its blocked on #35

[yazan-abdalrahman]

this does not seem to be complying to the specification.

however, I took a bit of a deeper look and one issue is we cannot update the testdata since its blocked on #35

@crowlKats

What is the specification that this change 'https://github.com/denoland/rust-urlpattern/pull/50' does not comply with?

This change 'https://github.com/denoland/rust-urlpattern/pull/50' does not change any tests; instead, it just updates the search field's regex to match any pattern when it has a default pattern of "no pattern."

[crowlKats]

@yazan-abdalrahman the specification is https://urlpattern.spec.whatwg.org. We shouldn't be adding odd behaviour as such to achieve desired outcomes. If what we are doing is wrong, and we don't match in behaviour of the spec, then we need to change our implementation. If instead chrome is doing things wrong, then chrome should either change or upstream their changes to the spec.

[yazan-abdalrahman]

@yazan-abdalrahman the specification is https://urlpattern.spec.whatwg.org. We shouldn't be adding odd behaviour as such to achieve desired outcomes. If what we are doing is wrong, and we don't match in behaviour of the spec, then we need to change our implementation. If instead chrome is doing things wrong, then chrome should either change or upstream their changes to the spec.

@crowlKats I debugged I didn't find another solution, best solution as I said first is to change the "parse constructor string" method and then update the test, I don't think this test is standard if u want to update them I can do it

  pub fn parse_constructor_string<R: RegExp>(
    pattern: &str,
    base_url: Option<Url>,
  ) -> Result<UrlPatternInit, Error> {
    let url = match Url::parse(pattern) {
      Ok(url) => url,
      Err(_) => {
        if let Some(base) = base_url {
          base.join(pattern).map_err(|_| Error::BaseUrlRequired)?
        } else {
          return Err(Error::BaseUrlRequired);
        }
      }
    };
    Ok(Self {
      protocol: Some(url.scheme().to_string()),
      username: if url.username().is_empty() {
        None
      } else {
        Some(url.username().to_string())
      },
      password: url.password().map(|s| s.to_string()),
      hostname: url.host_str().map(|s| s.to_string()),
      port: url.port().map(|p| p.to_string()),
      pathname: Some(url.path().to_string()),
      search: url.query().map(|s| s.to_string()),
      hash: url.fragment().map(|s| s.to_string()),
      base_url: None,
    })
  }

and this code it matches with specifications https://urlpattern.spec.whatwg.org/ Because it currently sets search as default to empty, not *, and another field to the same, and that does not match specifications, it should be changed tests to comply with the specification.

it has one difference in this implementation to setting port i can resolve it did u need to rewrite implementations and tests?

[yazan-abdalrahman]

@yazan-abdalrahman the specification is https://urlpattern.spec.whatwg.org. We shouldn't be adding odd behaviour as such to achieve desired outcomes. If what we are doing is wrong, and we don't match in behaviour of the spec, then we need to change our implementation. If instead chrome is doing things wrong, then chrome should either change or upstream their changes to the spec.

@crowlKats I debugged I didn't find another solution, best solution as I said first is to change the "parse constructor string" method and then update the test, I don't think this test is standard if u want to update them I can do it

  pub fn parse_constructor_string<R: RegExp>(
    pattern: &str,
    base_url: Option<Url>,
  ) -> Result<UrlPatternInit, Error> {
    let url = match Url::parse(pattern) {
      Ok(url) => url,
      Err(_) => {
        if let Some(base) = base_url {
          base.join(pattern).map_err(|_| Error::BaseUrlRequired)?
        } else {
          return Err(Error::BaseUrlRequired);
        }
      }
    };
    Ok(Self {
      protocol: Some(url.scheme().to_string()),
      username: if url.username().is_empty() {
        None
      } else {
        Some(url.username().to_string())
      },
      password: url.password().map(|s| s.to_string()),
      hostname: url.host_str().map(|s| s.to_string()),
      port: url.port().map(|p| p.to_string()),
      pathname: Some(url.path().to_string()),
      search: url.query().map(|s| s.to_string()),
      hash: url.fragment().map(|s| s.to_string()),
      base_url: None,
    })
  }

and this code it matches with specifications https://urlpattern.spec.whatwg.org/ Because it currently sets search as default to empty, not *, and another field to the same, and that does not match specifications, it should be changed tests to comply with the specification.

it has one difference in this implementation to setting port i can resolve it did u need to rewrite implementations and tests?

@crowlKats ?

[crowlKats]

Apologies about the delay @yazan-abdalrahman, was going through the backlog of urlpattern related PRs and issues. This was inadvertently fixed in #44 correctly, so will close this PR.