Skip to content
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

RFC: Use concrete types instead of generic type params? #87

Closed
raquo opened this issue Nov 19, 2022 · 20 comments
Closed

RFC: Use concrete types instead of generic type params? #87

raquo opened this issue Nov 19, 2022 · 20 comments

Comments

@raquo
Copy link
Owner

raquo commented Nov 19, 2022

Currently, the design of SDT type listings is maximally generic, providing a lot of flexibility to esoterically minded library authors.

The issues below mostly apply to all types of keys – props, attrs, css style props, and event props, but we'll be looking at specific event props as a concrete example:

trait MouseEventProps[
  EP[_ <: DomEvent],
  DomEvent,
  DomMouseEvent <: DomEvent,
  DomElementMouseEvent <: DomMouseEvent,
  DomDragEvent <: DomMouseEvent,
  DomWheelEvent <: DomMouseEvent
] { this: EventPropBuilder[EP, DomEvent] =>

    /**
    * The click event is raised when the user clicks on an element. The click
    * event will occur after the mousedown and mouseup events.
    *
    * MDN
    */
  lazy val onClick: EP[DomElementMouseEvent] = eventProp("click")

  // ... more props here

  /**
    * Fires when the mouse wheel rolls up or down over an element
    */
  lazy val onWheel: EP[DomWheelEvent] = eventProp("wheel")

  // ... more props here
}

A developer who uses a UI library like Laminar (or Outwatch or Calico) might see onWheel --> whatever in their code, and ctrl-click on the onWheel symbol in their IDE to go to its definition. The snippet above is where they will land.

The definition of onWheel has a short ScalaDoc comment, but there is no concrete information about the value we're dealing with. We don't know what this EP type is, or what shape DomWheelEvent is – these types are both abstract.


We could improve on DomWheelEvent by replacing it with org.scalajs.dom.WheelEvent – the concrete type that it resolves to for 99% of the users. Then users could proceed to ctrl-click into that type and see what kind of events they will get. This would improve ergonomics for most people. This benefit is obvious, the main issue is maintaining functionality for the other 1%, the use cases where we do want abstract type params.


Currently DomWheelEvent is an abstract type param because our trait MouseEventProps is in a shared project – it's cross-compiled for Scala.js and the JVM, and so it does not have access to the JS-only org.scalajs.dom types.

I designed it that way because I was hoping that Scala DOM Types would be used on the JVM as well, for example in tools that need to render HTML (web pages or email content) on the server. To my knowledge, as of today no such tools exist that use Scala DOM Types, but it's possible that they do exist, just that their authors are overly modest, and didn't let us know.

I also hoped that cross-compiling libraries would be using Scala DOM Types on both client and server, however, again, I'm not aware of any such libraries.

Also, @armanbilge pointed out that obviously there aren't enough IO monads in scalajs-dom, so he's looking into using our generic SDT design with a more FP-friendly alternative to scalajs-dom.


How could we provide concrete org.scalajs.dom types to the 99% of SDT users, while maintaining SDT's flexibility – one of our main design goals? I don't want developers to need to fork SDT or copy-paste our type listings into their project. Ideally there should be a single source of truth for SDT-style type listings in the Scala world. And thus it needs to be flexible enough for anyone to use.

I think one way or another, the answer has to involve code generation.

  1. We could keep existing type listings as-is in the shared project, with all their abstract params, and generate ScalaJS-specific copies of all those traits. A bunch of dumb stuff like replacing DomWheelEvent with dom.WheelEvent. Or, do it the other way, doesn't really matter. Laminar would use the ScalaJS version, other libraries which need more flexibility would use the shared project version with abstract type params. If someone is using both libraries in their ScalaJS project, redundant near-copies of SDT traits will be loaded into the JS bundle.

  2. Alternatively, we could keep some kind of "source of truth" for the type listings in the SDT project (whether in the shape of Scala traits, or something more structured and easier to parse), and have every consuming library (Laminar, Outwatch, etc.) generate their own bundle of DOM keys. We would need to provide a configurable generator for this, and I'm not quite sure how. Otherwise, same issues as option (1) in case you use more than one such library in your project, but there are some extra advantages too:

    a) In addition to making DomWheelEvent a concrete type, we could also make EP a concrete type, so in Laminar you would see a fully concrete type: lazy val onWheel: EventProp[dom.WheelEvent] = eventProp("wheel")

    b) Each library like Laminar could better customize what they want to generate, e.g. click vs onClick, typ vs `type`, etc. (this can be seen as a disadvantage too).

    c) For special types like DomElementMouseEvent (canonically TypedTargetMouseEvent), libraries would be able to choose the strategy they want, AND have that type be concrete too.

    d) Just like today, libraries could choose which traits they want to use, but the generator could also flatten the traits into a single fat trait, or into one trait per key type (props / attrs / etc.). Currently libraries need to create an object that mixes in 20+ traits. I have no specific insights, but I suspect that there are ways in which this is unhealthy in JS / ScalaJS.

    e) Libraries would be able to choose if they want val-s or lazy val-s (See Switch definitions from lazy vals to vals? #86)

    f) All the DOM types would be contained in the library project itself, no need to jump into SDT.

    g) Esoteric libraries that need something other than scalajs-dom would get the benefit of concrete types as well.

  3. We could "backport" some of the advantages of option (2) to option (1) if we generated a set of "blessed" SDT configurations inside SDT, but I'm not sure if that's a good tradeoff, e.g. EP type would still remain abstract.


Disadvantages of code generation:

  • The inelegance of redundant code, whether contained in SDT, or spread across several projects. There is no escape.

  • Working with generated code can sometimes feel like working with macros, i.e. "invisible code". We will avoid this by committing all generated code into github, so that all published code matches what you see on Maven Central, in the IDE, and on github. We do it this way in Airstream already.

  • Harder for users to figure out how to contribute


Examples of code generation:


Requesting comments!

I'm not yet sure which option is best, or whether the tradeoffs are worth it. There might be better options than what I've considered, or some other use cases I haven't realized. Any opinions or insights are welcome. Let me know what yall think.

Personal cc: @sjrd @keynmol @armanbilge @fdietze @cornerman @yurique @sherpal but the discussion is open to everyone.

@raquo raquo changed the title RFP: Use concrete types instead of generic type params? RFC: Use concrete types instead of generic type params? Nov 19, 2022
@armanbilge
Copy link
Contributor

Also, @armanbilge pointed out that obviously there aren't enough IO monads in scalajs-dom, so he's looking into using our generic SDT design with a more FP-friendly alternative to scalajs-dom.

To make it concrete, it looks like this:

https://github.com/armanbilge/calico/blob/671ff27cb87f3ddc73294f00d6b67e591a147d69/calico/src/main/scala/calico/html.scala#L61-L154

It's worth pointing out that although those may look like entirely different monadified types, secretly they are just opaque type aliases for the respective "raw" JS type.


So if I'm reading correctly, am I currently the only known user of this capability? 😅 Unless some other IO-based project like Outwatch or ff4s is also interested in what I'm doing, honestly you should leave me in the dust 😛


cc @buntec https://github.com/buntec/ff4s

@armanbilge
Copy link
Contributor

  • We could keep existing type listings as-is in the shared project, with all their abstract params, and generate ScalaJS-specific copies of all those traits. A bunch of dumb stuff like replacing DomWheelEvent with dom.WheelEvent. Or, do it the other way, doesn't really matter. Laminar would use the ScalaJS version, other libraries which need more flexibility would use the shared project version with abstract type params. If someone is using both libraries in their ScalaJS project, redundant near-copies of SDT traits will be loaded into the JS bundle.

If the scala-js-dom versions just extend the abstract traits and override every method with a call to super, I believe that would have the desired effect. It would be even cooler if that could be inlined away to avoid bloating the bundle, but I'm not sure about that.

@raquo
Copy link
Owner Author

raquo commented Nov 19, 2022

So if I'm reading correctly, am I currently the only known user of this capability? 😅 Unless some other IO-based project like Outwatch or ff4s is also interested in what I'm doing, honestly you should leave me in the dust 😛

It appears so with my limited knowledge, but I can't be sure, and I don't know if I'm ready to give up on the promise of flexibility just yet, both for the IO crowd and the backend tasks. Like if someone wanted to make a static site generator in Scala, they should be able to use SDT (although if I personally did it, I would probably run it on Laminar in node.js)

If the scala-js-dom versions just extend the abstract traits and override every method with a call to super, I believe that would have the desired effect. It would be even cooler if that could be inlined away to avoid bloating the bundle, but I'm not sure about that.

Right, I forgot to mention that approach. ScalaTags does it this way (possibly for different reasons).

In principle, both option (1) and option (2) could use such inheritance – just add extends domtypes.shared.TraitName[...] to the generated code – but I don't think there are significant advantages to inheritance. Calling super introduces indirection whereas we're trying to reduce indirection.

@armanbilge
Copy link
Contributor

Calling super introduces indirection whereas we're trying to reduce indirection.

Meh, turns out to be moot. super may be not be used on val

@davesmith00000
Copy link

Thanks for the cc @raquo - interesting discussion!

Also, @armanbilge pointed out that obviously there aren't enough IO monads in scalajs-dom

😂 If it's any consolation, he said that to me enough times that I put them in. 😉

I'm only on my first coffee of the day but I'm not totally following your point about the design being done the way it has been to allow usage on the JVM...? Tyrian's tags all work just fine on the JVM so that you can use them for SSR.... but I feel like I'm missing something? Perhaps for generating funky email content?

Anyway, I'm a massive fan of code generation these days, so I thought I might offer some thoughts on the disadvantages you called out based on my experience, but for the record I have no skin in this game so feel free to ignore me.

I seem to recall that I looked at using scala-dom-types for Tyrian because having "one Scala implementation" is indeed appealing, but the lifecycle differences between Laminar/Outwatch/Calico (FRP) vs Tyrian (Elm) are sufficiently different that it didn't feel like a good fit for me at the time.

The inelegance of redundant code

For performance and bundle size (I'm know you know, but for the casual reader) this makes no odds because Scala.js will strip all that stuff out, and on the JVM... what's a few KB between friends? So then it's just that every tag perhaps has a bulky definition to the reader, here is Tyrian's output for div:

  def div[M](attributes: Attr[M]*)(children: Elem[M]*): Html[M] =
    Tag("div", attributes.toList, children.toList)
  @targetName("div-list-repeated")
  def div[M](attributes: List[Attr[M]])(children: Elem[M]*): Html[M] =
    Tag("div", attributes, children.toList)
  @targetName("div-no_attrs-repeated")
  def div[M](children: Elem[M]*): Html[M] =
    Tag("div", Nil, children.toList)
  @targetName("div-repeated-list")
  def div[M](attributes: Attr[M]*)(children: List[Elem[M]]): Html[M] =
    Tag("div", attributes.toList, children)
  @targetName("div-list-list")
  def div[M](attributes: List[Attr[M]])(children: List[Elem[M]]): Html[M] =
    Tag("div", attributes, children)
  @targetName("div-no_attrs-list")
  def div[M](children: List[Elem[M]]): Html[M] =
    Tag("div", Nil, children)
  @targetName("div-repeated-list-plaintext")
  def div[M](attributes: Attr[M]*)(plainText: String): Html[M] =
    Tag("div", attributes.toList, List(text(plainText)))
  @targetName("div-list-list-plaintext")
  def div[M](attributes: List[Attr[M]])(plainText: String): Html[M] =
    Tag("div", attributes, List(text(plainText)))
  @targetName("div-no_attrs-list-plaintext")
  def div[M](plainText: String): Html[M] =
    Tag("div", Nil, List(text(plainText)))

All of those are really just alternate convenience constructors to make the syntax nicer for the user. It is a little noisy, but it is also so maintainable and the results work so nicely that the trade off is totally worth it in my view. If I find a bug, I fix it once and regenerate the nice, simple, concrete implementations for all.

Working with generated code can sometimes feel like working with macros, i.e. "invisible code".

I don't care about this one way or the other. Who among us is not using an IDE and cannot click though to the source in the lib (which you cannot do with macros)? The other thing is though, that generated code (at least, I've found) tends to trade off code cleverness/generic-ness for sheer volume. You don't need to write clever code in order to keep it terse and maintainable here, you can just output a lot of much simpler code that you wouldn't dream of maintaining by hand. So even though the code is invisible (to git, it exists in the jar), it's pretty simple - or at least the code I generate is. My point being that an IDE prompt and a compiler error are enough to get a user through it as there are no really clever definitions to absorb and reflect on.

I suppose though, if you were writing one shared implementation for everyone to use then having the code visible on github would be important. Maybe it's just a difference in use-case? 🤔

Harder for users to figure out how to contribute

I think this is the only real drawback in truth. Having said that, I've had a surprisingly large number of contributions in that area - maybe because Tyrian's code gen system is really quite simple? So I wouldn't be put off doing it again. Indeed, I am doing it again on a new project!

@raquo
Copy link
Owner Author

raquo commented Nov 19, 2022

Thanks for your comments!

not totally following your point about the design being done the way it has been to allow usage on the JVM...? Tyrian's tags all work just fine on the JVM

(For context – Laminar does not, and can not, run on the JVM, because it requires native JS DOM types)

In SDT / Laminar every tag has a unique "native JS element type" associated with it (to the extent that such unique types exist in JS / scalajs-dom) – for example, lazy val div is not just an HtmlTag, it's an HtmlTag[dom.html.Div], and lazy val input is HtmlTag[dom.html.Input]. Those types are useful on the frontend in Laminar because we don't have virtual dom, it's not uncommon to refer to the real DOM node.

However, under the original design constraints of Scala DOM Types, we can't put dom.html.Div or dom.html.Input in the definitions of div and input, because those scalajs-dom types do not exist on the JVM, and the div and input values needed to exist there too.

That's why I made those types into abstract type params instead. But now we have a billion type params in all the SDT traits, and all that indirection is hard for users to follow. I mean not necessarily conceptually, but practically it's hard to get to the definition of dom.html.Div type if you're looking at a usage of div in your code, because the definition of div only has abstract types in it. I guess the boring Div is a bad example, but there are also various weird element types and event types which you might want to look up.

I suspect some kind of JVM compatibility might be achieved by "stubbing" scalajs-dom types like dom.html.Div on the JVM, but I've largely ignored that approach as relatively resource intensive (to stub them all, and then keep up with changes to scalajs-dom) and not-very-useful since all you get are fake empty types. Also, wouldn't help with IO monads.

@fdietze
Copy link
Contributor

fdietze commented Nov 20, 2022

Thanks for the effort!

What is the source of truth for all those types currently?

@raquo
Copy link
Owner Author

raquo commented Nov 20, 2022 via email

raquo added a commit that referenced this issue Nov 28, 2022
…ion. Fixes #87.

This commit parses existing SDT traits into the new structures, and clean up scaladocs, and adds a bunch more documentation links.
@raquo
Copy link
Owner Author

raquo commented Nov 28, 2022

This is happening: #89

@fdietze
Copy link
Contributor

fdietze commented Nov 29, 2022

(sorry for the late reply, too many notifications...)

Do you mean where we bind abstract type params to concrete scalajs-dom types?

I was asking, where are the types originally came from? Did you enter them by hand, crawl them, etc. ?

If they could be generated from an official spec, it would be ideal. This is what I found after a quick search:

@raquo
Copy link
Owner Author

raquo commented Nov 29, 2022

Type definitions originally came from ScalaTags, and have been manually updated since. Not sure how they ended up in ScalaTags in the first place, but I suspect they were entered manually or semi-manually.

I'm not sure, but generating SDT types from official specs might not be feasible. Some concerns:

  • Need the ability to select which keys we want to crawl, and to group keys. Importing thousands of rarely used props might not be wise.
  • Need the ability to customize naming to follow Scala style and prevent conflicts
  • Need to translate any type information to our own type language
  • Need short descriptions for scaladoc comments, or the ability to provide them manually
  • Need the ability to add / edit keys due to bad or missing data in the source

When all is said and done, the crawler/generator will be pretty complicated, mostly due to the need to override generated data in various ways. Meanwhile, due to the backwards compatible nature of web standards, existing keys essentially never change, so If anyone wants to take this up, I suggest focusing on the smaller use case of generating a set of SDT KeyDef-s that you want to add (e.g. grid styles), and then committing the output to SDT. Such a crawler / generator might well live in SDT, but the point is, it should never be run again on the same set of keys, to allow for trivial edits / additions to the generated key defs, without needing to code complex mechanisms for that.

Hope that makes sense.

With that kind of scope in mind, I'm not opposed to adding such an importer to SDT, but I don't have the time to develop it myself.

@fdietze
Copy link
Contributor

fdietze commented Nov 29, 2022

I see. I don't have any objections here.

A few more references:

@raquo
Copy link
Owner Author

raquo commented Dec 3, 2022

Alright, this took way too much time, but it's done now. The generators are ahem not exceedingly elegant, but they work. Just finished integrating the changes into Laminar, and pretty happy with the result.

For reference:

Notable non-generated files in the latter commit:

  • Build setup – everything under project, including DomDefsGenerator.scala
    • Code generation is triggered at compilation time but only if a new version of scala dom types dependency is required (configurable)
  • Note that nothing from com.raquo.domtypes is used at runtime anymore, only used at build time now
    • ignore old code examples in websiteJs subproject, I just forgot to update them
  • Files under codecs and defs that previously lived in Scala DOM Types now live in Laminar:
    • Codecs
    • "complex" attributes (className / rel / role / dataAttr / styleAttr)
    • CSS style unit helpers (.px etc.) (optional)
    • These files are code, not really data / config, so trying to generate them is not worth the effort and complexity.
  • Notable changes to Laminar API
    • Simplified a bunch of types, removed abstract type params
    • Now you say documentEvents(_.onClick) instead of documentEvents.onClick
      • Side effect of removing abstract type param, but on the plus side, now you can say documentEvents(_.onClick.useCapture.preventDefault)
      • same for windowEvents

To be clear, I wanted to make those changes to Laminar's API – that's the whole reason I did all of that in the first place. But in your own library, you can still generate pretty much exactly the same traits as you currently use, you'll basicaly just need to configure the generator with different params, and copy over Codecs and the "complex" keys.

If anyone wants to give this a shot in your library:

  • See GeneratorSpec (and the resulting com.thirdparty package in the JS subproject) in Refactor everything in SDT for compile-time code generation #89
  • See Laminar's DomDefsGenerator as mentioned above
  • Customization options available to you, easiest ones first:
    • Provide different params to CanonicalGenerator's constructor
    • Provide different params to CanonicalGenerator's generate* methods
    • Instantiate generators manually instead of calling generate* methods
    • Override CanonicalGenerator's methods
    • Subclass individual trait generators and override their methods

Because the audience for this code is limited to a perhaps handful of experienced devs, and because we have two examples (GeneratorSpec and Laminar) to go off, I don't think I'll be writing any docs beyond putting a more polished version of that bullet list in the readme.

If you're stuck, or if it seems that the customization method required for your use case is disproportionately arduous, please let me know either here or in discord, and we'll figure it out.

Note: if you want to compile / run the new laminar tests locally, you need the latest commit from this SDT PR and also the latest commit from scala-dom-testutils master.

@raquo
Copy link
Owner Author

raquo commented Dec 3, 2022

One thing I'm not happy about is needing the sbt-buildinfo plugin in Laminar's build setup. It's used in build.sbt to read the ScalaDomTypes version defined in project/project/ProjectVersions.scala.

The file is nested in project's project because the SDT is listed as a dependency in project/build.sbt, because I think this is needed for me to be able to call into SDT's generator method from build.sbt.

It does work as-is, but I'm offensively bad at sbt, so it's quite possible that I made my own life harder than needed somehow.

@sjrd
Copy link

sjrd commented Dec 3, 2022

This looks great! :)

@yurique
Copy link
Sponsor Contributor

yurique commented Dec 3, 2022

Amazing! 👏 👏

What about dropping lazy val-s? I think I've seen it mentioned somewhere, in the context of scala.js now being able to strip unused val-s, right?

@raquo
Copy link
Owner Author

raquo commented Dec 3, 2022 via email

@raquo
Copy link
Owner Author

raquo commented Dec 4, 2022

Pushed updated README for library authors.

Note – it has a bunch of links to source files, but for now I haven't specified the URLs of those links, since the stuff isn't in master yet.

@cornerman
Copy link
Contributor

cornerman commented Dec 4, 2022

This is amazing! Thank you for all the work, you have put into this. I am super curious to try it out and build new customizations. It will be interesting to test lazy vals, vals and inline defs with the new code generator 👍

@raquo
Copy link
Owner Author

raquo commented Jan 3, 2023

I've just released Scala DOM Types 17.0.0-M1 with the new generators.

The README has instructions about integrating the new generator into your library. If you want to reference Laminar code for this, see the next-0.15 branch there.

I plan to cut 17.0.0 together with Laminar 15.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants