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

Addition: expose .toJSON() on GeolocationCoordinates + GeolocationPosition #147

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Apr 9, 2024

Closes #145

The following tasks have been completed:

Implementation commitment:


Preview | Diff

@reillyeon
Copy link
Member

I filed a Chromium issue to track this. Please file issues for WebKit and Gecko.

@marcoscaceres
Copy link
Member Author

Filed Gecko and WebKit bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1890706

@saschanaz, would you be the right person to comment on this?

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Apr 10, 2024

Sent a PR for WebKit WebKit/WebKit#27063

@marcoscaceres marcoscaceres changed the title Expose .toJSON() on GeolocationCoordinates Addition: expose .toJSON() on GeolocationCoordinates Apr 10, 2024
@saschanaz
Copy link
Member

👍 from Mozilla.

@marcoscaceres marcoscaceres changed the title Addition: expose .toJSON() on GeolocationCoordinates Addition: expose .toJSON() on GeolocationCoordinates + GeolocationPosition Apr 10, 2024
webkit-commit-queue pushed a commit to marcoscaceres/WebKit that referenced this pull request Apr 11, 2024
https://bugs.webkit.org/show_bug.cgi?id=272434
rdar://126183686

Reviewed by Ryosuke Niwa.

Implements and tests the toJSON() method for the GeolocationCoordinates interface, as proposed:
w3c/geolocation#147

* LayoutTests/fast/dom/Geolocation/coordinates-interface-toJSON-expected.txt: Added.
* LayoutTests/fast/dom/Geolocation/coordinates-interface-toJSON.html: Added.
* Source/WebCore/Modules/geolocation/GeolocationCoordinates.idl:

Canonical link: https://commits.webkit.org/277347@main
@marcoscaceres
Copy link
Member Author

@reillyeon, if all looks good, could you give it a ✅ 🙏

Patches ready to land on the WebKit side.

@reillyeon
Copy link
Member

I'm writing up the "Intent to Prototype and Ship" email for the Blink launch process and I just want to make sure I understand the motivation for this change. Are tests planning to use JSON.stringify() or is it that much easier to compare objects when they are "plain old JavaScript objects" than when they are instances of a WebIDL interface?

I noted when I was writing tests for this change in Chromium that the output of JSON.stringify() is not well defined (due to property order) so it doesn't seem appropriate for tests.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Apr 12, 2024

I'm writing up the "Intent to Prototype and Ship" email for the Blink launch process and I just want to make sure I understand the motivation for this change.

The primary motivation was to make it easier for developers to (re)use and serialize these objects.

await position = new Promise(r => navigator.geolocation.getCurrentPosition(r));
await fetch('https://example.com/api/positions', {
  method: 'POST', 
  headers: {
    'Content-Type': 'application/json'
  },
  body: JSON.stringify(position, null, 2)
});

And then it just happens that we can potentially reuse .coords.toJSON() for something like #146.

Are tests planning to use JSON.stringify() or is it that much easier to compare objects when they are "plain old JavaScript objects" than when they are instances of a WebIDL interface?

It's simpler to compare object's expected values than JSON.stringify() for the reasons you discovered.

I noted when I was writing tests for this change in Chromium that the output of JSON.stringify() is not well defined (due to property order) so it doesn't seem appropriate for tests.

Yeah, I concluded the same thing. You can see how I tested in WebKit:
https://github.com/WebKit/WebKit/pull/27063/files#diff-7531c0af803badf851f88a70c6702ee38c2936b1a42342e0430a3605befaf5b7

But the only interesting part is (overlook the fact that they are on the global object, that's just a WebKit testing framework thing):

          // We use "actual" for future proofing on purpose, in case more properties get added to the interface.
          for (const key in window.actual) {
            shouldBe(`window.actual.${key}`, `window.expected.${key}`);
          }

webkit-commit-queue pushed a commit to marcoscaceres/WebKit that referenced this pull request Apr 12, 2024
https://bugs.webkit.org/show_bug.cgi?id=272501
rdar://problem/126247408

Reviewed by Ryosuke Niwa.

Implements .toJSON() method on GeolocationPosition, as per spec change:
w3c/geolocation#147

* LayoutTests/fast/dom/Geolocation/position-interface-toJSON-expected.txt: Added.
* LayoutTests/fast/dom/Geolocation/position-interface-toJSON.html: Added.
* Source/WebCore/Modules/geolocation/GeolocationPosition.idl:

Canonical link: https://commits.webkit.org/277413@main
@saschanaz
Copy link
Member

Do we have context about why GeolocationPosition is not a dictionary? 🤔

@marcoscaceres
Copy link
Member Author

Do we have context about why GeolocationPosition is not a dictionary?

Just because of legacy, @saschanaz (given the API is like 10+ years old). If we were doing the API today, it would definitely just have been a dictionary.

@marcoscaceres marcoscaceres merged commit 09b48e6 into main Apr 15, 2024
2 checks passed
@marcoscaceres marcoscaceres deleted the toJSON branch April 15, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for converting Geolocation Position+Coordinates to JSON (object)
3 participants