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

Update to Scala Dom Types v17.0.0 #117

Merged
merged 90 commits into from
Jan 14, 2023
Merged

Conversation

2chilled
Copy link
Collaborator

@2chilled 2chilled commented Dec 4, 2022

Currently there is almost no support for HTML Global Attributes in html dsl. I quickly hacked together support for data-*, but I think it makes a lot of sense to support all of them properly. WDYT?

Also, what's the plan for fs2.dom types and their methods? Typecasting to underlying scalajs-dom types?

@armanbilge
Copy link
Owner

Currently there is almost no support for HTML Global Attributes

Hmm, I see. Are they missing from scala-dom-types? Would it make sense to add them there and implement them here?

Also, what's the plan for fs2.dom types and their methods? Typecasting to underlying scalajs-dom types?

Sorry, what's the question? This branch is already using fs2.dom types.

@2chilled
Copy link
Collaborator Author

2chilled commented Dec 5, 2022

Are they missing from scala-dom-types? Would it make sense to add them there and implement them here?

I think so. Some of them seem to be implemented already: https://github.com/raquo/scala-dom-types/search?q=Global_attributes

Sorry, what's the question? This branch is already using fs2.dom types.

Here's an example: https://github.com/armanbilge/calico/blob/pr/0.2-redesign/calico/src/main/scala/calico/html.scala#L262. One could get rid of casting by doing the wrapping within fs2-dom.

@armanbilge
Copy link
Owner

armanbilge commented Dec 5, 2022

One could get rid of casting by doing the wrapping within fs2-dom.

Hmm, I'm not sure what you mean.

The reason I use casting there (and many other places) is for performance. The problem is that a given with type parameters compiles to a def. So each time you use it, you would be allocating a new Modifier. Allocations are expensive.

Instead, I allocate the modifier exactly once as a val _forStringSignal. Then, the inline given can always return the same instance instead of allocating a new one, but it has to use a cast. This works due to type erasure.

@2chilled
Copy link
Collaborator Author

2chilled commented Dec 5, 2022

The reason I use casting there (and many other places) is for performance.

I see, but at the same time there's a cast from Modifier[F, dom.Node, Signal[F, String]] to [E <: fs2.dom.Node[F]]: Modifier[F, E, Signal[F, String]]. The implementation is in dom, not in fs2.dom. So the cast eliminates the whole need for fs2.dom to expose methods for fs2.dom.Node[F], because one can work in dom.Node and type cast into the opaque type.

Seen from a different angle, if fs2.dom.Node[F] would expose all the functions used on dom.Node here for example, one could replace Async[F] by Dom[F]. Performance optimization would still be possible via type cast.

I know, this doesn't really matter for calico as we're looking at internals, but I'm wondering what the strategy for fs2.dom is. Does this example qualify e.g. e.appendChild(dom.document.createTextNode(head)) and n.textContent = t to be exposed in an RT way from fs2.dom or not?

@armanbilge
Copy link
Owner

The implementation is in dom, not in fs2.dom.

Ah I see. In this case, that is also a performance optimization: e.appendChild(dom.document.createTextNode(head)) can be done in a single F.delay(...) instead of several chained effects if we used fs2-dom wrapper APIs.

I'm wondering what the strategy for fs2.dom is

Fair question. I would say that everything qualifies. Ideally, fs2-dom would wrap the entire DOM API safely. The challenges with this are:

  1. the entire DOM API is huge
  2. it's not always obvious the best API design to use
  3. there is a performance cost for wrappers

That's why I'm currently following a "lazy" approach, where I add new wrapper APIs based on practical needs and use. I think appendChild, createTextNode, and textContent would all be good additions.

@2chilled
Copy link
Collaborator Author

2chilled commented Dec 19, 2022

@armanbilge I started implementing an App based on this PR. Things are going quite well, but I noticed one strange behavior which I want to share with you, hoping you can spot something obvious. The minimized example is this:

import calico.html.Html
import calico.router.Router
import calico.router.Routes
import cats.effect.Concurrent
import cats.effect.kernel.Ref
import cats.effect.syntax.*
import cats.effect.syntax.all.*
import cats.Monad
import cats.syntax.all.*
import fs2.concurrent.SignallingRef
import org.http4s.*
import org.http4s.syntax.all.*

object ReloadBug:
  def apply[F[_]: Concurrent: Html: Monad: Router] =
    val html = summon[Html[F]]
    import html.{*, given}

    for {
      sigRef <- SignallingRef[F]
        .of[String]("init")
        .toResource
      routes <- routes(sigRef).toResource
      d <- div(
        cls <-- sigRef.map(_.pure[List]),
        summon[Router[F]].dispatch(routes),
        a(
          "Go to reloadbug2",
          onClick --> (_.foreach(_ => summon[Router[F]].navigate(uri"/reloadbug2")))
        ),
        a(
          "Go to reloadbug",
          onClick --> (_.foreach(_ => summon[Router[F]].navigate(uri"/reloadbug")))
        )
      )
    } yield d

  private def routes[F[_]: Concurrent: Html](ref: Ref[F, String]) =
    val html = summon[Html[F]]
    import html.{*, given}

    val r1 = Routes.one[F] {
      case u if u.path === uri"/reloadbug".path =>
        ()
    }(_ => ref.set("Set by reloadbug route").toResource >> h1("ReloadBug"))

    val r2 = Routes.one[F] {
      case u if u.path === uri"/reloadbug2".path =>
        ()
    }(_ => ref.set("Set by reloadbug2 route").toResource >> h1("ReloadBug"))

    (r1, r2).mapN(_ |+| _)

Once /reloadbug is initially loaded, div class is "init". I expected it to be "Set by reloadbug route". When clicking on the Links, div class is set by the corresponding routes as expected. What I know is, the signal change on initial load can be seen by <-- call, but somehow it doesn't get propagated to the actual dom element. Do you have an idea why this is? If not, I'll look deeper tomorrow.

@armanbilge
Copy link
Owner

I started implementing an App based on this PR. Things are going quite well

Awesome, great to hear!

but I noticed one strange behavior which I want to share with you, hoping you can spot something obvious.

Hmm, that does seem strange. Unfortunately I do not see anything obvious either, sorry 🤔

What I know is, the signal change on initial load can be seen by <-- call

How do you know this, did you add some sort of debug println ?


Btw, sorry things have been slow here. scala-dom-types is undergoing a major redesign in raquo/scala-dom-types#89 so we have a big upgrade coming up 😅

@2chilled
Copy link
Collaborator Author

How do you know this, did you add some sort of debug println ?

Exactly, when changing to

cls <-- sigRef.map { string =>
  println("Setting class to " + string)
  string.pure[List]
},

on initial load I can see this on the dev console:

Setting class to init
Setting class to Set by reloadbug route

but the actual class is still "init".

Btw, sorry things have been slow here. scala-dom-types is undergoing a major redesign in raquo/scala-dom-types#89 so we have a big upgrade coming up

Ah, no need to apologize :) I have plenty of work to do on this project before depending on snapshot deps becomes an issue.

@2chilled
Copy link
Collaborator Author

@armanbilge Even more strange, ad758c2 is a workaround..

Comment on lines 68 to 71
extension [F[_], A](signal: Signal[F, A])
private[calico] def getAndUpdates(using Concurrent[F]): Resource[F, (A, Stream[F, A])] =
// this hack makes me sad
Resource.eval(signal.get.tupleRight(signal.discrete.drop(1)))
Resource.eval(signal.get.tupleRight(signal.discrete))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see! Probably this hack is broken somehow 🤔

@2chilled
Copy link
Collaborator Author

2chilled commented Jan 2, 2023

@armanbilge First of all, happy new year :)

Next missing piece for me are SVG tags and attrs. Should I add them in the same way as I did for aria? This time however I'll also need to add some new Wrapper types to fs2-dom.

@armanbilge
Copy link
Owner

@2chilled happy new year! :)

Yes, you can start with fs2-dom. SvgElement will also have to be an opaque type since is in the same inheritance with HtmlElement.

For changes to Calico I feel a bit anxious until we can update to the new scala-dom-types. Have you had a chance to look at this PR?

If you have time, it would be good to experiment with how we will use that in Calico.

@2chilled
Copy link
Collaborator Author

2chilled commented Jan 2, 2023

SvgElement will also have to be an opaque type since is in the same inheritance with HtmlElement.

👍

For changes to Calico I feel a bit anxious until we can update to the new scala-dom-types. Have you had a chance to look at this PR?

I didn't look into details yet, but my hope is that we almost get a source compatible API to the current one. Difference is just that API is generated instead of hand-written, with the resulting "benefit" that there are less type params. But I can spend 1 or 2 hours tomorrow trying to lift 0.2-redesign branch into current SDT codegen SNAPSHOT, then we'll hopefully have more facts at hand.

@armanbilge
Copy link
Owner

but my hope is that we almost get a source compatible API to the current one. Difference is just that API is generated instead of hand-written, with the resulting "benefit" that there are less type params.

Yes, exactly, it should be entirely/nearly source-compatible :)

Since we have control over source-generation now, we may be able to do some additional improvements e.g. use Scala 3 inline modifiers. But we can explore that in a later step.

@2chilled
Copy link
Collaborator Author

2chilled commented Jan 3, 2023

@armanbilge I gave codegen a shot, see https://github.com/2chilled/calico/tree/pr/0.2-redesign-codegeneration

Unfortunately codegen cannot be used as is because the generators are not expressive enough in regards to kinds of generated type constructors. For example, to generate the constructor of HtmlTag the following generator is used:

s"def ${keyImplName}[$scalaJsElementTypeParam <: $baseScalaJsHtmlElementType](key: String, void: Boolean = false): ${keyKind}[$scalaJsElementTypeParam] = ${keyKindConstructor(keyKind)}(key, void)"

Obviously the kind of keyKind = HtmlTag is fixed to * -> *, which does not match our current calico.html.HtmlTag kind, which is (* -> *) -> * -> *, (* -> *) being the generic "effect type" F[_].

Therefore we need to either (1) make the SDT generators more expressive, or (2) try to push the effect types away from data definitions but into behavior (function) definitions. I guess (2) isn't really an option because we'd need to replace fs2-dom opaque types, since for these the effect types are necessary at data definition level. And without opaque types there is again allocation cost to pay, which we surely want to avoid.

As (1) seems to be the only option, let's look a bit deeper. Instead of generating something like:

trait HtmlTags {
  def htmlTag[Ref <: dom.html.Element](key: String, void: Boolean = false): HtmlTag[Ref] = new HtmlTag(key, void)

We'd need to be able to instruct the generator to generate something like this:

trait HtmlTags[F[_]] {
  def htmlTag[Ref <: fs2.dom.Element[F]](key: String, void: Boolean = false)(implicit F: Async[F]): HtmlTag[F, Ref] = new HtmlTag(key, void)

Note that I think we cannot use Scala 3 trait params, since AFAIK SDT understandably still wants to support Scala 2.

Last but not least this would have to be synced with @raquo, the maintainer of SDT. One could certainly keep the codegen API as it is today, and offer the effectful one separately, as I think the majority of SDT users won't have an interest to support first class effects. But even when @raquo agrees, this will be some work to implement.

@armanbilge
Copy link
Owner

@2chilled thank you very much for investigating that!

I thought we could keep a similar pattern by defining a protected method, which the super traits could call without needing to know about Async.

protected def htmlTag[E <: fs2.dom.HtmlElement[F]](tagName: String, void: Boolean) =
HtmlTag(tagName, void)


But actually, I think we will need to write our own source-generator in the end anyway. scala-dom-types relies on runtime casting in various places that won't work for our wrapper classes.

@2chilled
Copy link
Collaborator Author

2chilled commented Jan 3, 2023

I thought we could keep a similar pattern by defining a protected method, which the super traits could call without needing to know about Async

I see, but this would require the generator of that super trait to leave that method unimplemented, which is not the case ATM. Nevertheless, this would probably require less changes on SDT generator side, so good idea!

I think we will need to write our own source-generator in the end anyway. scala-dom-types relies on runtime casting in various places that won't work for our wrapper classes

Do you have an example of such a cast?

@armanbilge
Copy link
Owner

@armanbilge armanbilge closed this Jan 14, 2023
@armanbilge armanbilge reopened this Jan 14, 2023
@armanbilge armanbilge changed the title 0.2-redesign Update to Scala Dom Types v17.0.0 Jan 14, 2023
Copy link
Owner

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@2chilled thank you so much for all of your work on this 🙏 really relieved to make this upgrade 😅

Still there is quite a bit to do, so I created a milestone for v0.2.0. I will open a couple follow-up issues and please make your own!
https://github.com/armanbilge/calico/milestone/2

@armanbilge armanbilge merged commit 642b1b8 into armanbilge:main Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants