search

w3c / geolocation

W3C Geolocation API
https://www.w3.org/TR/geolocation/
81 stars 56 forks source link

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

Closed marcoscaceres closed 3 months ago

marcoscaceres commented 3 months ago

Closes #145

The following tasks have been completed:

Implementation commitment:

Preview | Diff

reillyeon commented 3 months ago

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

marcoscaceres commented 3 months ago

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 commented 3 months ago

Sent a PR for WebKit https://github.com/WebKit/WebKit/pull/27063

saschanaz commented 3 months ago

👍 from Mozilla.

marcoscaceres commented 3 months ago

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

Patches ready to land on the WebKit side.

reillyeon commented 3 months ago

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 commented 3 months ago

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}`);
          }
saschanaz commented 3 months ago

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

marcoscaceres commented 3 months ago

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.