w3c / push-api

Push API
https://w3c.github.io/push-api/
Other
144 stars 40 forks source link

Add bytes() method for reading bytes into a Uint8Array #370

Closed marcoscaceres closed 1 month ago

marcoscaceres commented 1 month ago

Closes #369

The Fetch API is getting a Uint8Array-returning bytes() method alongside its existing arrayBuffer() method, following the principle that APIs should generally vend byte buffers as Uint8Arrays.

This PR makes the same change for PushMessageData, which has its own distinct arrayBuffer method.

I'm assuming this is uncontroversial given the support from the three major implementations for doing this on Body, but I can open an issue and solicit explicit support separately if you'd prefer. I'll write tests if I get a signal that this is able to go forward.

It's unfortunate that getKey and applicationServerKey vend ArrayBuffers instead of Uint8Arrays, but it's too late to fix those now.

The following tasks have been completed:

Implementation commitment:


Preview | Diff

marcoscaceres commented 1 month ago

Filed Gecko bug.

marcoscaceres commented 1 month ago

@martinthomson, @beverloo, looking for interest from a second implementer (and no objections from the third).

evilpie commented 1 month ago

Does this have tests?

bakkot commented 1 month ago

I couldn't find any existing tests to extend here, presumably due to https://github.com/w3c/push-api/issues/365. So I haven't written any tests for this. Happy to do so if there's a way to do it.

saschanaz commented 1 month ago

I'm against any significant extension without having a test coverage, but this one is small enough that I guess it's fine given the situation.

marcoscaceres commented 1 month ago

We will at least get some limited testing just through the IDL interface. Sadly, most of Push is untested.

saschanaz commented 1 month ago

Yeah, having some coverage here is in my wishlist. Anyway consider this comment as +1 from Mozilla.

bakkot commented 1 month ago

With a +1 from Mozilla and an implementation in Webkit is there anything blocking this from landing? Does it need a statement of non-opposition from Chrome?

beverloo commented 1 month ago

We're happy with this, thank you! I'll try to get the change included in Chrome 128.

marcoscaceres commented 1 month ago

@bakkot

Does it need a statement of non-opposition from Chrome?

That's correct 👍 It's part of WebApp's working mode. We are all about Team Web! 🥰

marcoscaceres commented 1 month ago

implemented in Gecko, as per referenced Mozilla issue 🥳.