sherpal/LaminarSAPUI5Bindings

Discussions?

Closed this issue · 15 comments

This is really a discussion rather than an issue, I think.

In this example

def networkLinkHeaderSap(inPage: Signal[PageWithId], network: Signal[Option[SingleNetworkExport]]) = {

  val aStringSignal = networkToTitle(network)
  aStringSignal.map(s =>
  Link(
    _ => Title(
      _.level := TitleLevel.H2,
      _ => s
      // Would prefer ?
      //_ <-- aStringSignal
    ),
    _.href <-- inPage.map(p => s"Somewhere/networks/details/${p.id}")
    )
  )        
}

I want to make a title, which is a link. It all works very well... but there's a niggle... which is that commented part. What I think I'd want to write is;

def networkLinkHeaderSap(inPage: Signal[PageWithId], network: Signal[Option[SingleNetworkExport]]) = {
  Link(
    _ => Title(
      _.level := TitleLevel.H2,
       _ <-- networkToTitle(network)
    ),
    _.href <-- inPage.map(p => s"Somewhere/networks/details/${p.id}")
    )
  )        
}

In this case, it's not a big problem, but I could see things getting messy, if one wanted to compose multiple components in such a manner...

I think although am not 100% sure on terminology - what I'm asking is...

Can the Content part of <tag> Contents <tag> be an observable?

raquo commented

Perhaps what you're looking for is _ => child.text <-- aStringSignal (in the first snippet)? This would normally let you add dynamic text to an element in Laminar. I don't know how the underlying SAP UI library works, so I could be wrong, but I think it should work, if _ => s works.

@raquo Yup... that compiles :-).

I have to say that the combination is not all that I was hoping it would automagically be... I don't think SAP intend them to be composed this way! But still, that definitely works... so the design of this library is holding up very well.

Thankyou...

So, indeed the syntax _ => child.text <-- aStringSignal is not as nice as one would hope for.
This is the price to pay to allow all the other nicer syntax to access the specific attributes of these elements (for example when you do _.titleLevel := TitleLevel.H1 (by the way I made helper functions to not use these TitleLevel, but it's not yet published -- see on master).

A possibility could be to add, for each component a

import [...].{child => laminarChild}

object MyComponent {
  def child = laminarChild
}

and then we could do MyComponent(_.child.text <-- aStringSignal). Something to think about.

raquo commented

@sherpal Hm. I agree, I like the component level scoping that ModFunction provides, but needing _ => to escape that scoping to insert a regular modifier is annoying.

The _.child approach is interesting, and could be automated with a common trait, but there's quite a few similarly useful defs that you'd want to bring over from where child is defined (Laminar.scala). And then there are all the standard events and other props... I worry about naming conflicts with web component's own API, or just confusion regarding where the keys are coming from.

Alternatively, would it be possible to accept (ModFunction | Mod)* instead of ModFunction*? So that you could pass both component-scoped props with _.level := ... and global props with child <-- ... in the same apply call? Would type inference work for this, I wonder...

Indeed, it's tricky.
If type inference work, it's a good solution. If not, then maybe an implicit conversion could do the trick. If not, then defining apply as apply(mods: ModFunction*)(mods: Mod*) is also possible, although more ugly :D

Maybe another weird solution. In the PR filling the missing component, I made everything inherit from a base trait (because why not). And what we can do is make an implicit conversion between that type and L.

This would look like this:

trait WebComponent {
  val id: ReactiveProp[String, String] = idAttr
}

object WebComponent {
  import com.raquo.laminar.api.L
  implicit def webComponentToL(component: WebComponent): L.type = L
}

And now this allows to do MyComponent(_.child <-- stuffObservable) or MyComponent(_.img(...)).

That also removes the clashing problem because if a WebComponent has a property that clashes with an existing from L, the one from the WebComponent will take precedence because the compiler will not fetch the implicit conversion.

What do you guys think @raquo @Quafadas ?

sjrd commented

The (ModFunction | Mod)* seems to work:

$ cs launch scala:3.2.0-RC3
Welcome to Scala 3.2.0-RC3 (1.8.0_342, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> type ModFunction = String => Int
// defined alias type ModFunction = String => Int

scala> class Modifier(val s: String)
// defined class Modifier

scala> implicit def stringToModifier(x: String): Modifier = new Modifier(x)
there was 1 feature warning; re-run with -feature for details
1 warning found
def stringToModifier(x: String): Modifier

scala> def make(mods: (ModFunction | Modifier)*): Int = mods.map(mod => mod match { case mod: ModFunction => mod("bar"); case mod: Modifier => mod.s.length }).sum
1 warning found
-- Unchecked Warning: ----------------------------------------------------------
1 |def make(mods: (ModFunction | Modifier)*): Int = mods.map(mod => mod match { case mod: ModFunction => mod("bar"); case mod: Modifier => mod.s.length }).sum
  |                                                                                  ^
  |                the type test for ModFunction cannot be checked at runtime
def make(mods: (ModFunction | Modifier)*): Int

scala> make(_.length)
val res6: Int = 3

scala> make(_.length, new Modifier("babar"), "foobar")
val res7: Int = 14

Yes indeed it seems to work. Perhaps the best solution. I need to do some shenanigans to remove the warning but other than that it's ok:

def apply(mods: (ModFunction | Mod[ReactiveHtmlElement[Ref]])*): HtmlElement = tag(
    mods
      .map {
        case mod: Mod[_ >: ReactiveHtmlElement[Ref]]                      => (_: this.type) => mod
        case mod: Function[_ >: this.type, _ <: ReactiveHtmlElement[Ref]] => mod
      }
      .map(_(Carousel)): _*
  )

Fun fact (or perhaps a compiler bug 🤔 ): if we do this instead:

def apply(mods: (ModFunction | Mod[ReactiveHtmlElement[Ref]])*): HtmlElement = tag(
  mods
    .map {
      case mod: Mod[_ >: ReactiveHtmlElement[Ref]]                      => mod
      case mod: Function[_ >: this.type, _ <: ReactiveHtmlElement[Ref]] => mod(this)
    }: _*
)

It compiles but throws a org.scalajs.linker.runtime.UndefinedBehaviorError: java.lang.ClassCastException: com.raquo.laminar.modifiers.KeySetter@2 is not an instance of com.raquo.laminar.nodes.ReactiveHtmlElement at runtime.

sjrd commented

Well, you are playing with uncheckable matches, so you have to be a bit careful.

Probably. It's still confusing because both things seem to be equivalent. Apparently they are not.

Then, I'll try to make this to work on all demo examples and publish it if it does!

raquo commented

So uh... I must say I'm confused why that ClassCastException exception happens. Am I missing a bug in the implementation of that def apply? I understand that type params are uncheckable, but that's only a problem if you have false assumptions about them... right?

Anyway, I think ModFunction | Mod[ReactiveHtmlElement[Ref]])* approach is nicer, if you can make it work w/o warnings and exceptions. I think it's easier to understand than implicits, and I personally would prefer if only one val child existed, scoping a copy of it to some web component when it's semantically not scoped to it (it's a general purpose modifier after all) feels a bit wrong, although the idea and the _.child syntax is interesting / elegant imo.

sjrd commented

If there's no asInstanceOf anywhere, and the patmat doesn't report any warning (and @unchecked was not used), and there is a ClassCastException, it is probably a compiler bug in the patmat analysis. It's worth minimizing and reporting, if there isn't any existing bug for that yet. In this case the original problem seems to be that the match uses _ <: ReactiveHtmlElement[Ref] although it should at least be _ <: Mod[ReactiveHtmlElement[Ref]].

Why does the match use Function (java.util.function.Function?) with bounds instead of a scala.Function1 with direct arguments? Also why does the Mod case have _ >: even though the parameter has a stronger type Mod[ReactiveHtmlElement[Ref]]?

I suggest using:

def apply(mods: (ModFunction | Mod[ReactiveHtmlElement[Ref]])*): HtmlElement = tag(
  mods
    .map {
      case mod: Mod[ReactiveHtmlElement[Ref] @unchecked]                                  => mod
      case mod: Function1[this.type @unchecked, Mod[ReactiveHtmlElement[Ref]] @unchecked] => mod(this)
    }: _*
)

The @unchecked may not all be needed.

sjrd commented

You can probably factor this beast out as:

def applyModFunctions[WebCompObj, Ref <: dom.html.Element]](webCompObj: WebCompObj, mods: ((WebCompObj => Mod[ReactiveHtmlElement[Ref]]) | Mod[ReactiveHtmlElement[Ref]])*): Seq[Mod[ReactiveHtmlElement[Ref]]] = {
  mods
    .map {
      case mod: Mod[ReactiveHtmlElement[Ref] @unchecked]                                   => mod
      case mod: Function1[WebCompObj @unchecked, Mod[ReactiveHtmlElement[Ref]] @unchecked] => mod(this)
    }
}

and then use it in

def apply(mods: (ModFunction | Mod[ReactiveHtmlElement[Ref]])*): HtmlElement =
  tag(applyModFunctions(this, mods): _*)

I didn't do it on purpose but apparently in Predef there is a type alias type Function[-A, +B] = Function1[A, B], so it was indeed the Scala Function1. But perhaps I can use these @unchecked annotations.