Add type parameter for MessageEvent#data

[armanbilge]

So, I was motivated to do this because the lack of precision annoyed me when using the WebSocket APIs. But for the record I'm not totally convinced if it's a good trade-off in terms of usability vs correctness.

1. TypeScript uses a type parameter, but they never actually instantiate it anywhere.
2. In many cases Any is really as precise as you can be, anyway.
3. I couldn't really find solid docs/spec making guarantees about more precise types for MessageEvent#data. I did populate it where I could, based on my inference/understanding. But I could be totally wrong there 😆

[armanbilge]

Educated guess based on:

The server-side script that sends events needs to respond using the MIME type text/event-stream. Each notification is sent as a block of text terminated by a pair of newlines.

https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events#sending_events_from_the_server

[armanbilge]

Educated guess based on:
https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/send

[armanbilge]

Educated guess based on:
https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/send

[armanbilge]

I cheated here, because writing out the union is kind of annoying ...

[japgolly]

Hey @armanbilge. I've read through the patch and I think there's an oversight here. This PR basically does two things, right?

1. Add a type to MessageEventInit
2. Add a type to MessageEvent

Except if you look at both MessageEvent and Event, the init fields aren't exposed. Once the class has been created it's effectively a phantom-type that doesn't affect any further usage. I can't think a way that could be useful (where as I can definitely see the disruption it causes, even in this PR 😀). So I'd propose we wind it all back and just add a [_] to the init arg (which is gonna cause an unavoidable warning in Scala 3).

If we do that, this PR would just be adding a type to MessageEventInit. Is that actually useful? I can see some value there but it looks pretty tiny to me (eg. .data is typed but *Init classes are usually just write-once and discard).

WDYT?

[armanbilge]

Woooops 😅 I forgot to add the type parameter to .data in MessageEvent, which is where I intended to use it all along. So sorry! 😳

Does that affect your thinking on this?

[armanbilge]

There we go 😅

[japgolly]

Ah ok that makes sense. Hmmm, well the data field when using a websocket is going to be provided by the API (as opposed to using MessageEventInit) which means the type is going to be BufferSource or Blob or USVString which means you'd still need to do an asInstanceOf anyway (and not even a pattern-match unless you're handling externally and arbitrarily created WS messages). That's effectively the status quo anyway so personally I'm not seeing value worth the disruption to users' code, but WDYT? Do you agree or have a different perspective?

[armanbilge]

Hmm, why can't you pattern match? That's how I'm using it currently, and it would be nice to make the "unhandled" default case go away. Although, I'm not sure if js.| works like that 🤔

https://github.com/http4s/http4s-dom/blob/af4d02aa4d93ff692504132407ccdbf2eee00889/dom/src/main/scala/org/http4s/dom/WebSocketClient.scala#L135

Anyway, I was already lukewarm about it so if you're 👎 then yeah, let's just forget it :)

[japgolly]

Ah, you literally are handling externally and arbitrarily created WS messages 😂😂

In Scala 2 the patmat doesn't understand that there would be one case left unhanded. In fact, a vague part of my memory is telling me you'd need to change e.data match { to (e.data: Any) match { to make it work in Scala 2 properly, or maybe without a warning. Could be wrong though.

Anyway, I was already lukewarm about it so if you're 👎 then yeah, let's just forget it :)

Yeah I'm really on the fence. I love more precision in types! What about if we looked at other usage and tightened the types there? I saw one [String] and a lot of [Any]s. What if you were to dig into the specs more and narrow as many [Any]s as you could? I think if you get a good result there then that makes this an easy choice to approve.

[armanbilge]

In fact, a vague part of my memory is telling me you'd need to change e.data match { to (e.data: Any) match { to make it work in Scala 2 properly

Yup, I think I've done that before. So probably this change won't really help me anyway 🙃

What about if we looked at other usage and tightened the types there?

I combed through the spec while working on this and I'm pretty sure that we can't make it any more precise 😕 Going over some of the Anys:

• DedicatedWorkerGlobalScope.onmessage, which is the dual of postMessage which can be anything right?
  

  scala-js-dom/dom/src/main/scala/org/scalajs/dom/MessagePort.scala

  Line 31 in 088cdf0

  def postMessage(message: js.Any, transferList: js.UndefOr[js.Array[Transferable]] = js.native): Unit = js.native

• HTMLBodyElement.onmessage which seems inherited from WindowEventHandlers, which it's unclear to me if it can be any more specific?
  https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onmessage
• etc.

I'm no expert on the spec, but overall this seems like a lost cause.

[japgolly]

I'm no expert on the spec, but overall this seems like a lost cause.

Hmmm, yeah ok, agree. But thanks for looking into it! It didn't work out but it was a worthy goal to try 😊