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

Process a URLPatternInit doesn't call basic URL parser on base URL #242

Closed
shannonbooth opened this issue Jan 6, 2025 · 5 comments · Fixed by #250
Closed

Process a URLPatternInit doesn't call basic URL parser on base URL #242

shannonbooth opened this issue Jan 6, 2025 · 5 comments · Fixed by #250

Comments

@shannonbooth
Copy link

What is the issue with the URL Pattern Standard?

process a URLPatternInit does:

Set baseURL to the result of parsing init["baseURL"].

Which has blob handling, which does not seem required. This seems like it should be changed to invoke the basic URL parser instead: https://url.spec.whatwg.org/#concept-basic-url-parser

@shannonbooth
Copy link
Author

shannonbooth commented Jan 6, 2025

Another editorial problem I noticed in the same AO is that "process a URLPatternInit" does:

  1. Let baseHost be baseURL’s host.
  2. If baseHost is null, then set baseHost to the empty string.
  3. Set result["hostname"] to the result of processing a base URL string given baseHost and type.

But URL's host is a concept/variant, and is not a string. e.g it may be an ipv4 address which is a 32 bit integer. I believe the spec should be calling this on host's serialization instead (https://url.spec.whatwg.org/#concept-host-serializer). But I can split this into a different issue if preferred (I will probably run into a few of these type of things, I am implementing the spec from scratch).

@shannonbooth
Copy link
Author

shannonbooth commented Jan 8, 2025

One more I noticed I can also split out into a different issue, but putting it down here for reference.

Parse a pattern string (https://urlpattern.spec.whatwg.org/#parse-a-pattern-string), step 3.9.1 does:

  1. Set prefix be the result of running consume text given parser.

But not prefix is in scope, so I believe should be 'let'. Similar story for modifier token on step 3.9.6

@shannonbooth
Copy link
Author

Noticed another thing (I will do a pass over this afterwards and clean up this issue).

Canonicalizing an opaque pathname https://urlpattern.spec.whatwg.org/#canonicalize-an-opaque-pathname

On step 3. does:

  1. Set dummyURL’s path to the empty string.

But in this case the URL path is a list. So I guess this must means to set it to a list with one item of the empty string?

jeremyroman added a commit to jeremyroman/urlpattern that referenced this issue Jan 8, 2025
Blob handling is not required, since the resulting URL is not stored in
any way that would make the blob handling visible. This is consistent
with what the URL constructor and similar uses do.

This change should not be observable.

Partially addresses whatwg#242.
@jeremyroman
Copy link
Collaborator

re. canonicalize an opaque pathname, the URL path can be either a list of path segments (in the case of a non-opaque path) or a single path segment (in the case of an opaque path). As this is the latter case, I think the spec is correct.

Whether your implementation internally treats this case as a list containing a single path segment plus an opaque flag is up to you, but I believe this is intended to cause the basic URL parser's opaque path state (which expects this to be a string, not a list of strings) to operate correctly.

The other three things you've reported I agree with and have opened pull requests. Thank you very much for pointing them out.

@shannonbooth
Copy link
Author

Oh right, thanks! Yeah that last one was my misunderstanding!

jeremyroman added a commit that referenced this issue Jan 9, 2025
Use the basic URL parser when parsing URLs

Blob handling is not required, since the resulting URL is not stored in
any way that would make the blob handling visible. This is consistent
with what the URL constructor and similar uses do.

This change should not be observable.

Partially addresses #242.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants