Remove JavaScript realm dependency #219

yoshisatoyanagisawa · 2024-02-16T02:17:35Z

To allow the algorithms in the specification to be used outside of JavaScript, we need to remove dependency to JavaScript realm.

This CL make a URLPattern object associated with an underlying URL Pattern struct, and makes algorithms to use it instead.

Fixes #217

At least two implementers are interested (and none opposed):
- Google Chrome
- n/a (change is simply fixing an issue in a non-controversial way).
Tests are written and can be reviewed and commented upon at:
- n/a (this is a fix to how the spec express this, there is no behavior change, and existing tests cover this behavior)
Implementation bugs are filed:
- Chromium: n/a (no behavior change is required)
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1731418 (vendor does not yet implement the spec)
- WebKit: n/a (no reason to believe a change is required)
- Deno: n/a (no reason to believe a change is required)
- kenchris/urlpattern-polyfill: n/a (no reason to believe a change is required)
MDN issue is filed: n/a (change is on a spec detail not expressly documented)
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

To allow the algorithms in the specification to be used outside of JavaScript, we need to remove dependency to JavaScript realm. This CL make a URLPattern object associated with an underlying URL Pattern struct, and makes algorithms to use it instead. Fixes whatwg#217

yoshisatoyanagisawa · 2024-02-16T02:18:01Z

@jeremyroman @sisidovski @domenic Can I ask you to take a look?

spec.bs

- Run -> run - renamed |this| to |urlPattern|

sisidovski

To remove JavaScript realm, you also have to update 4.1. Integrating with JavaScript APIs not to accept realm and build URL Pattern with create?

yoshisatoyanagisawa · 2024-02-19T06:39:39Z

To remove JavaScript realm, you also have to update 4.1. Integrating with JavaScript APIs not to accept realm and build URL Pattern with create?

Yes. I tried to update them to eliminate realm dependencies.

sisidovski

LGTM

yoshisatoyanagisawa · 2024-02-21T04:46:32Z

Thanks for the review, @sisidovski.

@jeremyroman @domenic Will you take a look?

spec.bs

Co-authored-by: Domenic Denicola <[email protected]>

- The URLPattern class -> "URL patterns" - Introduce an "Introduction" subsection to contain the introductory text and examples - Introduce a "The URLPattern class" subsection to contain the IDL and method definitions - Introduce "The URL pattern struct" subsection to contain the struct definition - Introduce "High-level operations" to contain "create", "match", "has regexp groups" - Everything else can continue living in "Internals" and "Constructor string parsing" - Rename "Patterns" to "Pattern strings" to be slightly less confusing as a sibling of "URL patterns"

yoshisatoyanagisawa · 2024-02-22T10:28:26Z

@domenic Thanks for the review. I have addressed your comments.

spec.bs

jeremyroman · 2024-02-23T20:23:33Z

spec.bs

@@ -1973,23 +1983,22 @@ JavaScript APIs should accept all of:
 To accomplish this, specifications should accept {{URLPatternCompatible}} as an argument to an [=operation=] or [=dictionary member=], and process it using the following algorithm, using the appropriate [=environment settings object=]'s [=environment settings object/API base URL=] or equivalent.

 <div algorithm>
-  To <dfn export>build a {{URLPattern}} from a Web IDL value</dfn> {{URLPatternCompatible}} |input| given [=/URL=] |baseURL| and [=ECMAScript/realm=] |realm|, perform the following steps:
+  To <dfn export>build a [=URL pattern=] from a Web IDL value</dfn> {{URLPatternCompatible}} |input| given [=/URL=] |baseURL|, perform the following steps:


Hmm, this is a semantic change in the case where a URLPattern is provided -- it creates a second platform object wrapping the same URL pattern, rather than returning the existing one. If it were then exposed back to script (e.g., as the value of an attribute) it would no longer compare equal (===), its expandos would be visibly different, etc.

For the case where the author really does want a platform object because they might show it to script again, do we need to keep this? Should we have two versions of this algorithm, one which returns platform objects (and therefore takes a realm in case it needs to create them) and one which returns URL patterns?

Good catch. Do we know who the callers would be? I'm not sure the "returns a platform object" version would have any callers, currently.

But if we did have such a caller, then I agree a separate algorithm would be best.

For the SW static routing API usage, we actually do not provide the way to see data from JavaScript.
However, I do not have a good reason to block such use case from other APIs.
I tried to implement the algorithm with |realm| in addition.

Co-authored-by: Jeremy Roman <[email protected]>

domenic

This is looking really nice; great work.

spec.bs

domenic · 2024-02-26T03:58:13Z

spec.bs

@@ -1973,23 +1983,22 @@ JavaScript APIs should accept all of:
 To accomplish this, specifications should accept {{URLPatternCompatible}} as an argument to an [=operation=] or [=dictionary member=], and process it using the following algorithm, using the appropriate [=environment settings object=]'s [=environment settings object/API base URL=] or equivalent.

 <div algorithm>
-  To <dfn export>build a {{URLPattern}} from a Web IDL value</dfn> {{URLPatternCompatible}} |input| given [=/URL=] |baseURL| and [=ECMAScript/realm=] |realm|, perform the following steps:
+  To <dfn export>build a [=URL pattern=] from a Web IDL value</dfn> {{URLPatternCompatible}} |input| given [=/URL=] |baseURL|, perform the following steps:


Good catch. Do we know who the callers would be? I'm not sure the "returns a platform object" version would have any callers, currently.

But if we did have such a caller, then I agree a separate algorithm would be best.

spec.bs

Also, fixed the way to call [=create=] because it won't take |pattern| any more.

I changed like so by mistake. In this context, we would want to say that it is not relevant to create a URLPattern object as a JSON input. We need to say that because unlike JavaScript API, JSON API cannot take a URLPattern object itself, which has URLPatternOptions.

To allow the API to compare values in the API's attribute with '===', the way to use |realm| has been introduced.

spec.bs

Co-authored-by: Jeremy Roman <[email protected]>

yoshisatoyanagisawa · 2024-02-27T02:54:43Z

Thanks @jeremyroman for the review.
@domenic Will you take another look at the PR?

domenic

Amazing work!

spec.bs

Co-authored-by: Domenic Denicola <[email protected]>

yoshisatoyanagisawa · 2024-02-27T07:18:42Z

@domenic Thank you for the review!

Will somebody merge this PR?
Thank you in advance.

* Fix linking to URL Pattern spec after whatwg/urlpattern#219. * Fix ambiguous "credentials" link by defaulting it to Fetch. * Fix RFC 2119 compliance in a note.

Editorial: Various minor fixes * Fix linking to URL Pattern spec after whatwg/urlpattern#219. * Fix ambiguous "credentials" link by defaulting it to Fetch. * Fix RFC 2119 compliance in a note. Fixes #300.

yoshisatoyanagisawa mentioned this pull request Feb 16, 2024

Make the URLPattern algorithms independent from realm. yoshisatoyanagisawa/urlpattern#1

Closed

sisidovski reviewed Feb 16, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

yoshisatoyanagisawa added 2 commits February 16, 2024 04:33

Omit "struct" after [=URL Pattern=].

8fd2654

Make the create algorithm to avoid breaking the existing callers.

5baf900

yoshisatoyanagisawa mentioned this pull request Feb 16, 2024

ServiceWorker static routing API w3c/ServiceWorker#1701

Merged

sisidovski reviewed Feb 19, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

Addressed comments.

f2dc8dd

- Run -> run - renamed |this| to |urlPattern|

sisidovski reviewed Feb 19, 2024

View reviewed changes

Remove realm dependencies from other spec integrations

9ef78bf

sisidovski approved these changes Feb 21, 2024

View reviewed changes

domenic reviewed Feb 21, 2024

View reviewed changes

yoshisatoyanagisawa and others added 5 commits February 21, 2024 18:31

Update spec.bs

d188a8d

Co-authored-by: Domenic Denicola <[email protected]>

Rename [=URL Pattern=] to [=URL pattern=]

32acfe3

Another URLPattern to URL pattern rename.

52c1032

Move match as URL pattern's function

0d5c24d

Also updated has regexp groups

fa9383e

sisidovski mentioned this pull request Feb 22, 2024

Add docs for service worker static routing API mdn/mdn#527

Open

5 tasks

yoshisatoyanagisawa added 5 commits February 22, 2024 09:00

Use associated URL pattern instead of URLPattern itself in algorithms

a365f6d

Revise [=create=] to return a created [=URL pattern=]

46a8a05

Minor fix.

e7b873d

Make create exported

84f8a34

jeremyroman reviewed Feb 23, 2024

View reviewed changes

yoshisatoyanagisawa and others added 2 commits February 26, 2024 10:21

Update spec.bs

6a3c376

Co-authored-by: Jeremy Roman <[email protected]>

Update spec.bs

29ef4bd

Co-authored-by: Jeremy Roman <[email protected]>

yoshisatoyanagisawa added 2 commits February 26, 2024 01:30

Omit the beginning "A" from items.

4fa6e48

Hold on mentioning about {{URLPattern}} in the introduction.

8075951

domenic reviewed Feb 26, 2024

View reviewed changes

yoshisatoyanagisawa added 5 commits February 26, 2024 04:34

Remove ", which must be set upon creation."

9311ab2

Combine three steps.

d51ad85

Also, fixed the way to call [=create=] because it won't take |pattern| any more.

Rename from |urlpattern| to |urlPattern|

4e893ec

Implement the algorithm to build {{URLPattern}}

bf7a7c0

To allow the API to compare values in the API's attribute with '===', the way to use |realm| has been introduced.

jeremyroman approved these changes Feb 26, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

Update spec.bs

0699d52

Co-authored-by: Jeremy Roman <[email protected]>

domenic approved these changes Feb 27, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

Update spec.bs

62f5ffa

Co-authored-by: Domenic Denicola <[email protected]>

sisidovski merged commit 69ccf3a into whatwg:main Feb 27, 2024
2 checks passed

yoshisatoyanagisawa deleted the remove_realm_dependency branch February 27, 2024 07:41

jeremyroman mentioned this pull request Feb 28, 2024

Editorial: Various minor fixes WICG/nav-speculation#301

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove JavaScript realm dependency #219

Remove JavaScript realm dependency #219

yoshisatoyanagisawa commented Feb 16, 2024 •

edited by pr-preview bot

Loading

yoshisatoyanagisawa commented Feb 16, 2024

sisidovski left a comment

yoshisatoyanagisawa commented Feb 19, 2024

sisidovski left a comment

yoshisatoyanagisawa commented Feb 21, 2024

yoshisatoyanagisawa commented Feb 22, 2024

jeremyroman Feb 23, 2024

domenic Feb 26, 2024

yoshisatoyanagisawa Feb 26, 2024

domenic left a comment

domenic Feb 26, 2024

yoshisatoyanagisawa commented Feb 27, 2024

domenic left a comment

yoshisatoyanagisawa commented Feb 27, 2024 •

edited

Loading

Remove JavaScript realm dependency #219

Remove JavaScript realm dependency #219

Conversation

yoshisatoyanagisawa commented Feb 16, 2024 • edited by pr-preview bot Loading

yoshisatoyanagisawa commented Feb 16, 2024

sisidovski left a comment

Choose a reason for hiding this comment

yoshisatoyanagisawa commented Feb 19, 2024

sisidovski left a comment

Choose a reason for hiding this comment

yoshisatoyanagisawa commented Feb 21, 2024

yoshisatoyanagisawa commented Feb 22, 2024

jeremyroman Feb 23, 2024

Choose a reason for hiding this comment

domenic Feb 26, 2024

Choose a reason for hiding this comment

yoshisatoyanagisawa Feb 26, 2024

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

domenic Feb 26, 2024

Choose a reason for hiding this comment

yoshisatoyanagisawa commented Feb 27, 2024

domenic left a comment

Choose a reason for hiding this comment

yoshisatoyanagisawa commented Feb 27, 2024 • edited Loading

yoshisatoyanagisawa commented Feb 16, 2024 •

edited by pr-preview bot

Loading

yoshisatoyanagisawa commented Feb 27, 2024 •

edited

Loading