search

scala-js / scala-js-dom

Statically typed DOM API for Scala.js
Other
318 stars 162 forks source link

Add type parameter for `MessageEvent#data` #687

Closed armanbilge closed 2 years ago

armanbilge commented 2 years ago

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 😆
japgolly commented 2 years ago

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 commented 2 years ago

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?

japgolly commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 :-1: 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 commented 2 years ago

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:

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

japgolly commented 2 years ago

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 😊