-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Null handling #481
Comments
Yes, I ran into this too when reviewing a PR. The idiom here so far seems to be not to indicate the possibly |
Btw, #414 (comment) made me think critically about what the purpose of this library is. I don't mean to open a can-of-worms over |
I agree with that sentence but I don't agree that clarifying nulls in types goes beyond that scope. In the same way the Plus I'd also argue that in addition to simply being pure facades, we also provide utilities for convenient Scala usage, and there's already precedent for that. (Check out the |
Re #414, yeah I don't know about generating this code from TS. Seems like a great idea if everything's up-to-date, and we have a means of overriding certain generated decisions. It's interesting but I'd also say it's orthogonal to the goals/purpose of this project. Like regardless of whether we hand-write or generate the code in this repo, we still need to make decisions about how to make the facades as precise as possible, and ensure they have good-enough dev ergonomics, so I'm happy to separate the decisions. |
So, why not return |
|
Plus different subtle semantics too, like you can't represent |
Right, but as long as we rule out nested I guess the question I'm asking is, what's the difference between having a facade that uses |
We could have an implicit conversion from The reason I'm wondering about |
Apologies, perhaps I'm misunderstanding, but |
Right, of course it doesn't go on the facade itself :) absolutely not! But in theory this library could provide a higher level interface with |
I haven't thought deeply about the solution, I'm just clarifying the problem and the limitations here :) But yeah if we end up with just a type alias over |
See my comment above about implicit conversions ;) |
Although you make it good point, it's an implicit conversion to add syntax rather than an implicit conversion to another type. Important distinction, my bad! |
That's a nightmare! As an example scalajs-react is mostly that, the facades aren't as safe and nice to use as typical Scala stuff, so here's a better layer on top of that, and it's the most time-consuming library to maintain that I have. Maybe I'm misunderstanding you but if you're suggesting having double the types with one always performing minor conversions to the other, I don't think that's the way to go. |
I agree, now I see better where you're going with this! |
No no you're right to highlight that! I think the former is much more acceptable than the latter, and we shouldn't rule out any kind of implicit. Thanks for clarifying. So like I said I haven't put any thought into the solution yet but no implicit would be better than syntax via implicits, which would be better than implicit type conversion. |
So is |
They are :P see here https://github.com/scala-js/scala-js/blob/8a0655b2ddf3047c13d49814c4debb7f10b88931/library/src/main/scala/scala/scalajs/js/Union.scala#L108 |
Which sadly means for us, they cannot be, because we do not have access to the |
Oh! That's cheating!! In the union companion object! |
Alright we'll figure something out. There are always ways hehehe. We should make our own companion object! |
I think you are right, this would work! |
eg idea: sealed trait NullOr[+A] extends js.Any
object NullOr { magic here } |
I think it could also be |
I feel like I've tried that before and it didn't work but happy to try again. |
package org.scalajs.dom
import scala.scalajs.js
import scala.scalajs.js.|
import scala.annotation.unchecked.uncheckedVariance
sealed trait NullOr1[+A] extends js.Any
object NullOr1 {
@inline implicit final class Ops[A](private val self: NullOr1[A]) extends AnyVal {
@inline def asUnion: A | Null = self.asInstanceOf[A | Null]
def toOption: Option[A] = if (self eq null) None else Some(self.asInstanceOf[A])
}
}
object ModuleNullOr2 {
type NullOr2[+A] = (A @uncheckedVariance) | Null
object NullOr2 {
@inline implicit final class NullOr2Ops[A](private val self: NullOr2[A]) extends AnyVal {
def toOption: Option[A] = if (self eq null) None else Some(self.asInstanceOf[A])
}
}
} package external
trait NullOr1Test {
import org.scalajs.dom.NullOr1
def x1: NullOr1[Int]
def y1 = x1.toOption
}
trait NullOr2Test {
import org.scalajs.dom.ModuleNullOr2._
def x2: NullOr2[Int]
def y2 = x2.toOption
} objects aren't implicit scopes for type aliases sadly:
|
@lihaoyi Thank you for the history, definitely helps me to better understand and appreciate the library as it is today.
👍 I like this proposal.
I also think this is definitely worth experimenting with. Would you mind specifying some of your quality concerns, so I can get a better idea of where an auto-generation scheme needs improvement? See also #486 (comment). |
The original code generator was a pair of Scala programs, one that translated typescript definition files to Scala code, another that scraped https://developer.mozilla.org/en-US/docs/Web/API to fill in the scaladoc. Both were half-baked, and generated broken code in many cases. They were also never source controlled, and the typescript definition generator has been lost to the mists of time, although the doc scraper I managed to fish out of my archive https://gist.github.com/lihaoyi/86eba5e0956861350b5b98bbb87e6516. Basically, it didn't work well at all. But it was a weekend project from someone new to Scala back when Scala was younger and Scala.js was still pre-release. I'm sure with infrastructure like ScalablyTyped to build upon you'd be able to get a much higher level of quality than I did, though it remains to be seen whether it'll be enough to match the current hand-maintained facades. The current ones are really pretty good, but who knows maybe @oyvindberg's code generator is just as good!. If you can get code-gen working well, then it'll also make it trivial to code-gen wrappers, e.g. if someone wants a different encoding for nulls, we could just tweak the code-generator and re-generate. That's probably the path forward if we want to easily experiment with different encodings and wrappers, as manually wrapping the thousands of DOM APIs everytime we want to try something new is totally infeasible |
I have no doubt that code generation can get us very good results. As you allude to @lihaoyi once we have the code generator with full knowledge about all types, sky is the limit for the what we can generate. For now a lot of effort has been spent on generating boilerplate to use third party react components in an effortless way, see for instance usage of the antd component library in a scalajs-react demo . It's easy to envision code transformations which for instance could move all members of javascript types to extension methods and translate However, let's not go overboard here. scala-js-dom is at the top of most projects' dependency trees for pretty much any Scala.js project. One of the huge strengths of the Scala.js ecosystem is its unparalleled stability and strong compatibility story over time. Reiterating what I said in #486 (comment), I'd personally favor a conservative direction for scala-js-dom. It has very few problems now, and any interested party can voluntarily try a more experimental DOM wrapper. My two cents anyway |
Thanks all, this was very informative for me :) I see the value in preserving what we have here while possibly augmenting with copy-pasta from ST, very similar to how this library was born too as far as I can tell.
@japgolly I do agree with this, that's definitely a tricky thing with your |
I'm trying to stay away from computers this weekend but let me just quickly
say: this is a community project, and community is not only valuable on its
own right, but also increases net value over time. Just cos I'm a
maintainer now doesn't mean it's my way or the highway; if a fair
representation of the community isn't onboard with something we should
probably drop it. I still want to push for this because I believe it's in
my, and everyone's best interests, and I still want to sketch up my ideas
and have a more concrete debate, but at the end of the day if it's not the
way to go then we won't. The process is a bit time consuming but I believe
that properly considering ambitious ideas is the best way to operate, even
if they don't end up being implemented. Null handling and code generation
are two areas that could be of great benefit after an initial cost has been
paid. Should we open a new issue to discuss code gen? As I see it is
orthogonal to null handling.
…On Fri, 13 Aug 2021, 11:34 pm Arman Bilge, ***@***.***> wrote:
Thanks all, this was very informative for me :) I see the value in
preserving what we have here while possibly augmenting with copy-pasta from
ST, very similar to how this library was born too as far as I can tell.
That's probably the path forward if we want to easily experiment with
different encodings and wrappers, as manually wrapping the thousands of DOM
APIs everytime we want to try something new is totally infeasible
@japgolly <https://github.com/japgolly> I do agree with this, that's
definitely a tricky thing with your NullOr proposal as it would be a lot
of work to go through the source and find all the places we need to make
this change. At the very least, a 2.x issue (this issue is currently in 1.x
milestone).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#481 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRRN2QH3HLJLTSEU5IZ6DT4UNMVANCNFSM5CCP4QWQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
💯 thrilled to be working with someone who has this philosophy 😁
👍 but imho we shouldn't put this in a 1.2 release but definitely consider it for 2.0.
I will do this. |
Instead of an issue, I made "discussion" #487 for code gen, hopefully the discussion format will work better for this topic! |
Here's a quick sketch of my proposal: import scala.scalajs.js
import scala.scalajs.js.|
sealed trait NullOr[+A] extends js.Any
object NullOr {
@inline def apply[A](a: A): NullOr[A] =
a.asInstanceOf[NullOr[A]]
@inline def empty: NullOr[Nothing] =
null
// Non-commutative. Do properly later.
@inline def fromUnion[A](a: A | Null): NullOr[A] =
a.asInstanceOf[NullOr[A]]
@inline implicit final class Ops[A](private val self: NullOr[A]) extends AnyVal {
@inline def asUnion: A | Null =
self.asInstanceOf[A | Null]
@inline final def get: A =
self.asInstanceOf[A]
@inline final def toOption: Option[A] =
if (self eq null) None else Some(get)
// ++ all the same methods as exists in js.UndefOrOps
}
} and some example usage @js.native
object SomeFacade extends js.Object {
val blah: NullOr[Int] = js.native
}
def usageExample() = {
SomeFacade.blah.toOption: Option[Int]
SomeFacade.blah.asUnion : Int | Null
SomeFacade.blah.get : Int
SomeFacade.blah.orNull : Integer
} I think this is great
@sjrd @lihaoyi does seeing this clarify my original intent? Would you still have concerns with this kind of solution? |
This proposal looks straightforward to me! Curious to hear @sjrd and @lihaoyi's thoughts.
I think this is still true ... I'm just not sure why it's so important, that we can't/shouldn't have |
@japgolly I think it clarifies the original intent, but I don't think it really alleviates my concerns. Really, it comes down to not wanting the "raw" API for scala-js-dom to have anything that's not already widely used and ubiquitous. Nulls, for better or worse, are widely used in both Javascript and JVM and ubiquitous to both platforms. There's a time and place for experimenting with new encodings and new core data types, and I don't think it should be in the most widely depended on Scala.js package in the entire ecosystem. If As I said above, I think the path for trying novel ways to improve things with significant breakage (and source incompatibility with every single null-returning DOM API is a very significant breakage!) is to play around in the |
Another idea, what if |
@japgolly yes, having it be part of scala-js itself would make me ok with it being in scala-js-dom, but I can't speak to the technical reasons for whether that would be a good idea or not Moving it out to a mini library nobody else is using does not address my concerns. I care about existing adoption and standards, not about how we organize jars and poms. If it was a mini library many people were already using across the ecosystem, then I'd be ok with it. As i've said elsewhere, scala-js-dom is effectively part of the Scala.js standard library. Literally everyone depends on it, often through deep dependency trees. It should be the last to adopt any new innovation, not the first! Let application code with zero dependencies be the first, and other libraries be second. scala-js-dom should be the last in line for adoption, after the consequences and characteristics are widely known and any unexpected issues have been worked out.
I'm not opposed to you trying this out, but you can try it out without breaking anyone by doing it in an |
I guess I just have a very different view of risk and change so we're probably not going to see eye to eye. That being said I'd like to keep exploring this discussion a little longer and see what comes out of it. See for me, I'm not greatly worried about risk here because no one's gonna die if we end up with an improvement but not The Best Improvement In The World, and the risk mitigation as I see it, should it be necessary, is literally, just a little migration script.
Why? That's not at all axiomatic or universally agreed upon. We can already clearly see that this is an improvement upon the status quo. Why should a majorly used library be the last to get an improvement? The whole fear here seems to be "what if we come up with a different encoding in the future". Like we know how to handle those types of migrations with minimal effort to users, we have the tools, so again: why should improvement only come from the fringe edges and not from the core?
2.0 will have breaking changes regardless, plus this is a binary-compatible change, just not source compatible.
The underlying details may change but the API wouldn't. The API would be pretty much exactly the same as
This seems reactionary rather than well considered to me. The API would be stable, we can tinker with the implementation under the hood if we need to without breaking compatibility. What would "disaster" really be in this case? I'd agree with your words 100% in other cases but the point I'm trying to stress here is that to me, this specific case when you really consider it concretely, doesn't seem to warrant these types of concerns which are normally valid.
I hadn't considered half-way, (or maybe "both world" is a better word), solutions, but that does open up new doors. The first two ideas that come to mind:
Any other ideas for good compromises? |
Playing devil's advocate: a reasonable counter-argument to my "hey what's a little migration if needed" is that the Scala community has put up with an insane amount of required downstream maintenance over the years, and one the points of Scala 3 that I'm starting to appreciate the most, is that upgrading everything after minor Scala versions will be a thing of the past. So maybe maintenance-fatigue is a fair critique? |
I don't see it as a clear improvement, maybe a sideways change. It's stricter, but also more verbose, and makes Scala.js diverge from both Scala-JVM and Javascript itself in how platform nulls are handled. Not obvious to me that it's an improvement on the status quo.
The total amount of migration work on each breaking change is proportional to the number of downstream projects. Thus it makes sense to try and minimize the churn in the most upstream projects: this is normally done by introducing new ideas in the most downstream projects where churn is cheap (change the API, no dependents to migrate!) and letting them stabilize before introducing them into the more upstream projects (change the API, lots of dependents to migrate) In this case we aren't going to be the one paying the full migration cost, since we don't maintain all the downstream projects, but it still exists in the community. I know a lot of people complain about churn and breaking changes in the Scala ecosystem, and the maintenance burden of publishing libraries; this is our chance not to unnecessarily exacerbate that problem. Separately, the more upstream a project is, the more likely diamond dependencies become an issue. I don't actually know how diamond dependencies work on Scala.js, but on Scala-JVM they are always a headache
I'm not sure what tools you've been using to do minimal effort migrations, but I do plenty of migrations at work with large Scala-JVM dependency trees, and we spend time (hours-days-weeks) wrestling with breaking changes and diamond dependency issues on an ongoing basis. And the more upstream the dependency, the bigger the headache. As a user of a lot of Scala libraries, I'm definitely not seeing this "minimal effort" haha But I think I've said my piece, can see what others think |
Yes! I think you are already there actually :)
The BIG win here is if the experimental version is kept 100% binary compatible.
Here's how we fix this:
I think this gets us 90% of the way there. Two caveats I can think of:
I hope this could be a reasonable compromise, thoughts? |
I feel like the "less is more" applies here and perhaps delaying this until 3.x has been out awhile and this lib has been published under new management. Like many, I also tried a bunch of ways and I don't think I ever liked any of them enough to feel strongly that there was an obvious choice. |
Wow. I have no words. If this is honestly the way that some people view this situation then fine, but I'm done putting in any more effort towards it, at least for now. I'm not gonna waste time trying to convince people that "nulls are bad and no-nulls good". If we can't even agree on that then this is by far a lost cause.
Seriously, just wow. |
@japgolly I'm hopeful that once the community sees this library is back on track with the basics (i.e., merging PRs and releasing regularly) it will inspire more engagement and support for advancement and feature development :) |
@armanbilge Yeah maybe. I'll just close this for now, we can always re-open it later. |
This thread popped back up into my mind today and I just wanted to say sorry @lihaoyi for the way I responded earlier. I'm kind of going through some really hard times this year and I let some misplaced emotion rule and I can see now that the way I responded above is actually pretty rude. I shouldn't have responded to you like that, and I shouldn't have made this space oddly-heated so I'm also really sorry to the entire thread. I'm such a huge proponent of big, warm, welcoming, accepting, environments and upon reflection, my behaviour here did the exact opposite. I'll keep striving to be better but for now I just wanted to say sorry :) |
Yeah no problem at all! This is an inevitable part of dealing with new
collaborations, I'm just happy to have you guys on board regardless of any
disagreements :)
…On Fri, 27 Aug 2021 at 2:35 PM, David Barri ***@***.***> wrote:
This thread popped back up into my mind today and I just wanted to say
sorry @lihaoyi <https://github.com/lihaoyi> for the way I responded
earlier. I'm kind of going through some really hard times this year and I
let some misplaced emotion rule and I can see now that the way I responded
above is actually pretty rude. I shouldn't have responded to you like that,
and I shouldn't have made this space oddly-heated so I'm also really sorry
to the entire thread. I'm such a huge proponent of big, warm, welcoming,
accepting, environments and upon reflection, my behaviour here did the
exact opposite. I'll do better but for now I just wanted to say sorry :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#481 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHEB7HWZIT42G7SMCHBG3TT64WZXANCNFSM5CCP4QWQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
What do we do in cases where a type in facade is nullable? We have
js.UndefOr
but sadly there's nojs.NullOr
. In scalajs-react I add| Null
to facade types but the problem is it's non-trivial to get rid of the null (outside of Scala 3's experimental explicit-nulls), and I've had to add helpers with casts.I absolutely hate the idea of just saying
AudioTrack
and the user having to somehow know that it's nullable. I want more clarity for users: types would be the best, information is probably the fallback (eg. annotations, worse: doc).What do to? It might be an idea to add something like
NullOr
to scala-js-dom itself.The text was updated successfully, but these errors were encountered: