scala-js / scala-js-dom

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

Null handling #481

Closed japgolly closed 3 years ago

japgolly commented 3 years ago

What do we do in cases where a type in facade is nullable? We have js.UndefOr but sadly there's no js.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.

japgolly commented 3 years ago

Would @sjrd be willing to add something like this to Scala.js itself? Say it was accepted into Scala.js 1.8.0 and we base scala-js-dom 2.0.0 on it, that sounds like it would alleviate your concerns, right @lihaoyi ?

japgolly commented 3 years ago

If NullOr was already in broad usage, part of the standard library like UnderOr is, a common idiom for other Scala.js code, or commonly used for other facades e.g. on ScalablyTyped, then I'd be more willing to use it here. But none of these is currently the case.

Another idea, what if NullOr was published as a mini-library and we depend on it for 2.0+. We'd end up going first but then presumably other projects and facades would start using it too. The end result would be the same as what you're describing, it's just that this could be the first bit of mainstream use. And to be clear, I wouldn't be making this argument for anything radical, it's just that a find NullOr so universally useful and trivial in its implementation, so personally I'm finding those properties acceptable tradeoffs in this specific case.

lihaoyi commented 3 years ago

@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.

NullOr's definition is useful and trivial. Such a large breakage across the entire Scala.js ecosystem (a lot of DOM APIs can return nulls!) is very much not trivial. Neither are the unexpected consequences and edge cases; if we end up having to iterate on NullOr as part of scala-js-dom, that means N large breakages across the entire Scala.js ecosystem, with multiple incompatible versions floating around, and people getting random versions resolved based on their dependency tree. That would be a disaster.

I'm not opposed to you trying this out, but you can try it out without breaking anyone by doing it in an .ext package, iterate to your hearts contents, and people can adopt when they're ready as it naturally stabilizes. Even without a specialized code generation tool, it should be pretty easy to set up SBT to copy-paste the source folder and regex-mangle some definitions so we can easily keep both versions in sync without additional maintenance overhead. It would be a bit of build-tool hackiness, but that definitely beats widespread breakage and incompatibility

japgolly commented 3 years ago

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.

It should be the last to adopt any new innovation, not the first!

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?

Such a large breakage across the entire Scala.js ecosystem (a lot of DOM APIs can return nulls!) is very much not trivial

2.0 will have breaking changes regardless, plus this is a binary-compatible change, just not source compatible.

unexpected consequences and edge cases; if we end up having to iterate on NullOr as part of scala-js-dom

The underlying details may change but the API wouldn't. The API would be pretty much exactly the same as UndefOr which is already prior-art.

multiple incompatible versions floating around, and people getting random versions resolved based on their dependency tree. That would be a disaster

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'm not opposed to you trying this out, but you can try it out without breaking anyone by doing it in an .ext package

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?

japgolly commented 3 years ago

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?

lihaoyi commented 3 years ago

We can already clearly see that this is an improvement upon the status quo.

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.

Why should a majorly used library be the last to get an improvement?

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

Like we know how to handle those types of migrations with minimal effort to users, we have the tools

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

armanbilge commented 3 years ago

Any other ideas for good compromises?

Yes! I think you are already there actually :)

Two distinct modules (eg. scala-js-dom & scala-js-dom-experimental-or-something-similar) in which the API in the later module is exactly the same with the exception of nullable types (which also gives binary compatibility between both.

The BIG win here is if the experimental version is kept 100% binary compatible.

Would have to think about the implications of both being on the classpath + transitive composition via libraries)

Here's how we fix this:

  1. for a library, scala-js-dom-experimental should NEVER be a dependency that is inherited transitively by downstreams. If you wish to use it (internally), add it as a compile-time-only (provided?) dependency.
  2. If you transitively depend on scala-js-dom in your classpath, but prefer to use the syntax available in scala-js-dom-experimental, then simply exclude scala-js-dom and add a dependency to scala-js-dom-experimental following rule (1) as needed.

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?

aappddeevv commented 3 years ago

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.

japgolly commented 3 years ago

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.

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.

Not obvious to me that it's an improvement on the status quo.

Seriously, just wow.

armanbilge commented 3 years ago

@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 :)

japgolly commented 3 years ago

@armanbilge Yeah maybe. I'll just close this for now, we can always re-open it later.

japgolly commented 3 years ago

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 :)

lihaoyi commented 3 years ago

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 https://github.com/scala-js/scala-js-dom/issues/481#issuecomment-906961810, 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.