`val` or `def` for read-only properties in facades?
armanbilge opened this issue · 7 comments
Two parts to this.
- Decide our convention and document in #523.
- Fix inconsistencies in the current codebase.
My only concern is that just because a JS property is read-only publicly, doesn't mean its value never changes, it can still be a privately mutable variable with a public read-only accessor. If we use a val in the facade and the browser modifies the value between calls, can that result in any bugs where some Scala optimisation just refers to the previous value without re-retrieving the current value? @sjrd what's your take on this?
We could just make everything a def and never use val; it might be a bit crappy in that a downstream user wont know by looking at the facade whether the underlying value can change, but it would avoid us having to constantly try to track down the distinction, and maintain it everywhere. With a def it's always going to be correct, just maybe not as precise as it could be.
Right, that's a really important point.
The difference between a
valand adefwithout parentheses is that the result of the former is stable (in Scala semantics). Pragmatically, usevalif the result will always be the same (e.g., document), anddefwhen subsequent accesses to the field might return a different value (e.g., innerWidth).
Via https://www.scala-js.org/doc/interoperability/facade-types.html.
I read this as it doesn't make a difference at runtime. Still, safest is to make them all def.
I read this as it doesn't make a difference at runtime
Oh I didn't. I read it as describing the same as I did, but with no indication of the impact of the semantics difference.
Still, safest is to make them all
def
Yeah I agree. I don't want to spend my life perfecting every nuance of this library (unless I'm being sponsored). I think it would be quite fair to have a few practices that reduce our maintenance costs so long as it doesn't have a significant impact on users. To be fair, it's also for this same pragmatic reason that I also don't mind there being inconsistency around this. I'd be happy with either.
Actually, come to think of it, there are certain causes like document, window, console etc that would clearly be vals and would actually be weird if they were defs. Dude maybe we should just accept this inconsistency. It's going to be inconsistent regardless.
Dude maybe we should just accept this inconsistency. It's going to be inconsistent regardless.
Fair point. Ok, IDK if it's worth mentioning in contributing docs, but at least let's be mindful of this when reviewing.
The policy would be something like "def unless you have good reason it should be a val" I think.
Yeah very good point. I'll add that to the doc soon
Documented in #523.