Use scala-dom-types #62

cornerman · 2017-10-15T22:43:14Z

We could use https://github.com/raquo/scala-dom-types for building vnode and just hook our builders. This way we could get rid of all the html definitions in the code.

This is not entirely done. As we currently cannot mixin Tags, Attributes and Events together into one object like we do with dom. After disucssing with the maintainer, this might get fixed. But we need to consider that we will anyhow never be able to mixin Styles and Properties into it, because they will conflict with the Attributes.

What do you think?

raquo · 2017-10-16T04:23:07Z

@cornerman I've addressed some of the issues in raquo/scala-dom-types#1

I didn't have the time to test it with my own projects yet, but it should work. One thing that I still haven't resolved yet is how to deal with naming collisions. To outline the problem:

We want one object (e.g. outwatch.dom) contain all attrs / props / styles / tags that we are interested in.
Some of those keys have conflicting names (e.g. value attr vs prop, color attr vs style, etc.)

I haven't yet checked how many collisions like this exist – it's kinda hard because Scala DOM Types listings were originally borrowed from ScalaTags, which specifically doesn't have all the attrs / props because of name conflicts, e.g. ScalaTags does not have a value prop, or a color attribute.

I'm not yet sure how to address this, but this seems like something that should be addressed at the Scala DOM Types level. Not including such basic things as a value prop is unacceptable for such a project I think.

Some of the potential solutions:

Rename the less-used conflicting key, e.g. color attr to colorAttr. There's already precedent for this with heightAttr, widthAttr, and formAttr, inherited from ScalaTags. Very simple, but I'm not sure if I want to mangle names just because in some use cases conflicts between props and attrs must be avoided. Also, it's hard to decide whether valueAttr or valueProp (or both?) should get the lame name. One other advantage is that IDE autocompletion will help you find both options as yo start typing valu.... I think if the number of conflicts is very low, this would be a good approach. In case of value specifically it would be nice to prompt end users to think whether they want an attr or a prop given the subtle but important differences between those.
Move conflicting keys into separate traits and let the users (dependent libraries) sort it out in any way they want. So we would have e.g. an ExtraAttr trait that would contain the color and value attrs. You could then use those methods with different names in Outwatch, but libraries that don't bundle all keys into one object will just include ExtraAttrs together with other attrs. The downside is that the bundling use case will become rather verbose.
In addition to (2), provide ExtraKeys trait where conflicting keys live and are accessible using attrs.value, props.value, etc. This trait would look like this:

trait ExtraKeys {
  val attrs: ExtraAttrs // trait mentioned in (2) above
  val props: ExtraProps // similar but for props
}

I kinda like this approach because it will be very flexible – you don't need to use ExtraKeys trait if you don't ant to, and you're free to bundle ExtraAttrs and ExtraProps any way you want. However, using ExtraKeys will require users to specify attrs.value or props.value depending on what they want (instead of just value), and is kinda hard to discover for the end user if they don't know about ExtraKeys or don't remember which keys it contains.

instead of valueAttr and valueProp we can make syntax like value.attr or value.prop work but the implementation on Scala DOM Types would probably be somewhat ugly and not flexible.

I'm definitely open to ideas & feedback on this.

raquo · 2017-10-16T04:38:28Z

@cornerman Oh, one other issue is how Scala DOM Types deals with Styles (I mean the object x extends Style parts of it). That pattern is inherited from ScalaTags, and I'm not particularly emotionally attached to it, but as of now I also don't know a better way.

The way it's implemented right now, I think you will need an implicit conversion from Scala DOM Types' Style to Outwatch's StyleBuilder. I use a similar pattern in my projects to provide the := syntax.

cornerman · 2017-10-16T16:54:24Z

@raquo Nice, thank you for working on this. The changes for the builders look good to me; will try it out later!

For the styles, I can live with the current design, it should be doable with an implicit conversion as you say. The advantage of doing it like ScalaTags is that the implementations are similar and fixes on one side can be merged easier. I'll let you know if I have a better idea.

cornerman · 2017-10-16T17:04:26Z

@raquo Another question regarding boolean attributes:

In html there are two different kinds of boolean attributes. Quoting from the HTML5 spec: "A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value." But, for example draggable is not a boolean attribute, but an enumeration of true|false. The latter attributes cannot be rendered like boolean attributes and need an explicit value (see also #53).

In outwatch, we therefore have two different builders for attributes with boolean values. One that emits a boolean value and another one that emits a boolean value as a string. Thereby, we can hand the value downstreams and snabbdom will decide how to render the attribute depending on the type of the value. Attributes with a boolean value (strictly equal to true or false) will be rendered as boolean attributes.

In scala-dom-types, both kinds of attributes are handled with a boolean value and there is no way on our end to decide which builder to use. Any ideas how to handle this? How do you do it?

For example draggable is built the same way as hidden, but both need to be rendered differently.

LukaJCB · 2017-10-16T17:21:20Z

I haven't had a lot of time to look at all of what this PR has to offer, but I'd just like to say thanks for all of your efforts, all of you! Scala-dom-types definitely seems like a great library (so does Laminar btw) so I hope we can leverage it

cornerman · 2017-10-16T17:55:49Z

@raquo Maybe another option for (not) handling conflicts in dom-types:

This mentions that changes to attributes are always synced into the properties and vice versa ("with some exceptions"). Also when using pure html, I am never confronted with these kinds of conflicting names, because html only has attributes. Styles are also not conflicting as they live inside their own attribute (style="..."). So, maybe we should not even mix Attributes and Properties together? When constructing html, I just want to use attributes.

Only for the special cases (like input.value - are there others?) we need to explicitly set the property when updating the attribute (like done in outwatch code). But this is another story and does not affect the dom types. It only suggests, that using only attributes is enough.

So, the option would be to just leave scala-dom-types traits as is and let them conflict. In outwatch, we could just mixin the Tags, Attributes and Events. For the Properties and Styles, we would just have a trait and an object. And you can inherit, import and shadow values as you like. In the normal use case, the import outwatch.dom._ should suffice.

raquo · 2017-10-17T07:45:07Z

@cornerman Re: pseudobooleans – Interesting quirk.

A few ways to deal with it that I see:

Make such cases A[Option[Boolean]] instead of A[Boolean]. The None case would then be mapped to absence of the attribute, and true / false to corresponding string values. The benefit is that Option[Boolean] is a distinct type from Boolean, however it is also subject to type erasure, which might or might not be a problem in some use cases.
Make some sealed trait X which includes three objects – True, False, and... Auto I guess? And type attrs like draggable as Attr[X]. This seems kinda ugly though, what with non-standard True / False types. And it could need implicit conversions for a convenient API (e.g. not only Bool -> X, but also possibly Stream[Bool] -> Stream[X])?
Look at how SDT deals with void tags in generic.builders.TagBuilder:

def tag[Ref <: DomElement](tagName: String, void: Boolean): T[Ref]

Maybe we could do something similar for attributes – just add another attr method:

def attr[V](key: String, stringifyValue: Boolean): A[V]

(stringifyValue would be true for pseudoboolean attrs like draggable)

Or maybe even limit availability of that second method to only boolean attrs, but that seems a bit overkill?

def attr[V](key: String, stringifyValue: Boolean)(implicit someBooleanEvidence: ???): A[V]

I think I prefer option (3). Would this work for Outwatch? If so, do you need / want the implicit evidence?

There's another issue lingering here – if you set draggable to true, how do you "unset" it – not set it to false, but rather remove the attribute altogether to get the "auto" value? I think the answer is not specific to these pseudo-boolean attributes, it should be some other method in the same place where := is defined (like your :=?), so we don't need to address this in SDT.

I will have more time in a couple days to think and implement this.

cornerman · 2017-10-17T16:37:12Z

@raquo Thanks for the response, I would also prefer option (3). I think an implicit evidence would be cool to limit the availability of this function - as it is only applicable for boolean. So, I think this should work for outwatch.

For the other options, I do not really like to switch the type of the attribute, as it is still a boolean. The difference lies in the attribute and not in the value I am assigning (i.e. draggable is rendered differently than hidden, but as a developer I do not really care and just want to enable/disable it). Also, with option (1): How would differentiate between a true boolean attribute and a true enumeration attribute. The value of the boolean attribute should not be rendered. So, more like Either[Boolean,Boolean]?

With regards to the "unset"-issue. At least for outwatch, I do not see a problem. Snabbdom handles this for us already and interprets attributes with boolean values correctly. The value "true" creates an attribute without value and the value false removes the attribute from the node. Am I overlooking something?

Btw. how do you currently handle this in snabbdom?

raquo · 2017-10-21T07:40:26Z

@cornerman I ended up implementing this a bit differently, in a way that lets you differentiate boolean vs enumerated attrs by either type or implementation (or both), and without requiring implicits. There's a new booleanAttr method on AttrBuilder. That one should return pure boolean attrs (those where true / false regulates presence / absence).

I've now added proper typings for boolean / enumerated attrs, and also for all aria attrs.

Btw. how do you currently handle this in snabbdom?

I was not aware of this pseudoboolean attrs problem, so it must be broken. I'll fix it in Snabbdom.scala after publishing a version of SDT that makes this distinction.

fdietze · 2017-10-22T20:14:53Z

I just rebased this PR on master.

@LukaJCB since a lot of code got removed, please check that we didn't remove any custom outwatch attributes you introduced (like inputString etc.).

cornerman · 2017-10-24T17:10:13Z

@raquo Thank you so much! This is really nice to use now.

One more point: We are currently using Dynamic for having a nicer syntax for data-attribute builders. So you can write something like this: div(data.foo.bar := "baz") (data-foo-bar = "baz"). Currently, this is not possible with domtypes, because data is defined as def data(name: String) requiring a name and does not return a different type as the other builders. Do you think, it makes sense to make a special case for the data attribute? We could add another type parameter to the AttributeBuilder make data a value. But then, we also get a conflict with the data-tag :)

As of now, we have just added an additional builder Data, which also works. So you can either write data("foo-bar") := "" or Data.foo.bar := "".

raquo · 2017-10-24T17:54:23Z

@cornerman I've actually renamed data to dataAttr (didn't commit this yet), so you will be able to go back to using data as Dynamic. SDT itself can't use Dynamic because it needs to stay compatible with the JVM, but you can definitely add your own attributes like data on top.

raquo · 2017-10-24T18:03:05Z

Oh, and I will also rename data tag to dataTag, it's way too uncommon to deserve such a generic name.

cornerman · 2017-10-25T17:34:41Z

@raquo Dynamic is not a scalajs feature, but also works on the JVM: http://www.scala-lang.org/api/current/scala/Dynamic.html. Anyway, with your renaming this is fine, then :)

fdietze · 2017-11-08T14:07:26Z

I just rebased this branch on top of master. Everything compiles and the tests pass. (If one published scala-dom-types 0.2.2-SNAPSHOT locally)

The next step is to port it to scala-dom-types 0.3:

"com.raquo" %%% "domtypes" % "0.3"

Currently it produces these compile-errors:

[info] Compiling 2 Scala sources to /outwatch/target/scala-2.12/classes...
[error] /outwatch/src/main/scala/outwatch/dom/DomTypes.scala:48: wrong number of type arguments for com.raquo.domtypes.generic.defs.attrs.Attrs, should be 1
[error]   extends Attrs[AttributeBuilder, BoolAttributeBuilder]
[error]           ^
[error] /outwatch/src/main/scala/outwatch/dom/DomTypes.scala:49: not found: type InputAttrs
[error]   with InputAttrs[AttributeBuilder, BoolAttributeBuilder]
[error]        ^
[error] /outwatch/src/main/scala/outwatch/dom/DomTypes.scala:50: not found: type GlobalAttrs
[error]   with GlobalAttrs[AttributeBuilder, BoolAttributeBuilder]
[error]        ^
[error] /outwatch/src/main/scala/outwatch/dom/DomTypes.scala:51: wrong number of type arguments for com.raquo.domtypes.generic.builders.AttrBuilder, should be 1
[error]   with AttrBuilder[AttributeBuilder, BoolAttributeBuilder] {
[error]        ^
[error] four errors found

cornerman · 2017-11-10T00:34:28Z

src/main/scala/outwatch/dom/DomTypes.scala

+    propCodec: Codec[V, DomPropV]
+  ): AttributeBuilder[V] =
+    new AttributeBuilder(attrKey, CodecBuilder.encodeAttribute(attrCodec))
+    //or: new PropertyBuilder(propKey, CodecBuilder.encodeProperty(propCodec))


Here, we can decide whether we want to represent the reflected attributes as attributes or properties. See https://github.com/raquo/scala-dom-types#reflected-attributes

cornerman · 2017-11-10T00:37:31Z

src/main/scala/outwatch/dom/OutwatchAttributes.scala

+
+/** Snabbdom Key Attribute */
+trait SnabbdomKeyAttributes {
+  //TODO are these necessary?


Do we need this?

cornerman · 2017-11-10T00:50:38Z

src/main/scala/outwatch/dom/DomTypes.scala

+    //The BooleanAsAttrPresenceCodec does not play well with snabbdom. it
+    //encodes true as "" and false as null, whereas snabbdom needs true/false
+    //of type boolean (not string) for toggling the presence of the attribute.
+    case _: BooleanAsAttrPresenceCodec.type => (v: V) => v.asInstanceOf[Boolean] //TODO


this is still TODO! we just need to ignore the BooleanAsPresenceCodec.

cornerman · 2017-11-24T01:24:13Z

Check attributes in snabbdom, accepting string | number | boolean:
https://github.com/snabbdom/snabbdom/blob/master/src/modules/attributes.ts#L4

and properties in snabbdom, accepting any value:
https://github.com/snabbdom/snabbdom/blob/master/src/modules/props.ts#L4

So i think, Props needs to accept Any

cornerman · 2017-11-24T10:38:31Z

src/main/scala/outwatch/dom/DomTypes.scala

-      case _ => codec.encode _
+    // snabbdom needs true/false of type boolean (not string) for toggling the presence of the attribute.
+    def encodeAttribute[V](codec: Codec[V, String]): V => Attr.Value = {
+      case b: Boolean => b


This is not correct. We would interpret every boolean value as a boolean attribute. But only attributes with a BooleanAsAttrPresenceCodec codec should be rendered like this.

Forr example contenteditable is not a boolean attribute, but an attribute with an enumerated value (true|false).

Edit: I have added some test for this.

Yes, I missed that. Thanks for adding the test.

LukaJCB · 2017-11-24T16:26:46Z

I haven't looked at everything in full detail yet, but this looks really good so far and I trust you guys to know what you're doing! 👍

mariusmuja · 2017-11-24T16:27:48Z

src/main/scala/outwatch/dom/Compat.scala


-  lazy val `for`: RA[String, String] = forId
+  @deprecated("Use forId instead", "0.11.0")
+  lazy val `for` = forId


Why should we deprecate this? Maybe scala-dom-types could include it and also `class`? It already includes `type`.

@raquo

Would be fine with me, I already opened a PR for class, but maybe we can even have both? :)

mariusmuja · 2017-11-24T16:32:18Z

src/main/scala/outwatch/dom/Compat.scala

+  lazy val inputNumber = onInputNumber
+
+  @deprecated("Use onInputChecked instead", "0.11.0")
+  lazy val inputChecked = onInputChecked


Good idea to add these for backwards compatibility! I think we should add a few other commonly used attrs/props, such as: click, keydown,...

You are right! i guess input and change are also very popular, but there meaning definitely changed because of the lack of InputEvent (same as for createInputEventHandler()).

Hmm... yes. I don't think we can alias input and change because of the different meaning. Probably better to get a compilation error in that case.

cornerman · 2017-11-24T16:41:32Z

src/main/scala/outwatch/dom/Compat.scala

 import org.scalajs.dom.{ClipboardEvent, DragEvent, KeyboardEvent, MouseEvent}
-import outwatch.dom.helpers.InputEvent
+
+import scala.language.higherKinds


These imports are not necessary, as we have all language features enabled via a compiler flag

That's correct. Intellij is was not smart enough to realise that :)

mariusmuja · 2017-11-24T18:36:41Z

Added some attribute aliases for backwards compatibility. This allows for example the demo-app in outwatch-extras to compile without change when this is merged.

What other attributes/tags should we alias with deprecation warnings?

cornerman · 2017-11-25T08:59:25Z

src/main/scala/outwatch/dom/Compat.scala

+
+trait AttributesCompat {
+  lazy val `class` = className


Which attributes should be deprecated? I kind of like the idea to mainly rely on scala-dom-types defined names, but I guess we can make some exceptions: Do we want to keep for and class or should we deprecate them? I think it should be either both or none. What do you think?

decided to keep for and class. the rest is deprecated.

fixes outwatch#20, outwatch#49

fixes outwatch#89

cornerman · 2017-11-25T21:42:36Z

@raquo Thank you so much for helping in integrating scala-dom-types into outwatch. Great work!

fdietze · 2017-11-26T14:05:45Z

The next step would be to advertise scala-dom-types to other libraries, like scalatags. 😉

raquo mentioned this pull request Oct 16, 2017

Make API more flexible (specifically, for Outwatch) raquo/scala-dom-types#1

Merged

cornerman force-pushed the scala-dom-types branch from 116dbce to 99b0b20 Compare October 21, 2017 19:56

fdietze force-pushed the scala-dom-types branch from 99b0b20 to e3fd5fe Compare October 22, 2017 20:12

fdietze changed the title ~~[Do not merge] Proposal to use scala-dom-types~~ Use scala-dom-types Oct 22, 2017

fdietze force-pushed the scala-dom-types branch from e3fd5fe to 95435fc Compare October 24, 2017 22:23

fdietze force-pushed the scala-dom-types branch from 95435fc to 70dd6fa Compare October 26, 2017 15:54

fdietze mentioned this pull request Nov 5, 2017

Provide separate streams for input.value and onChange #71

Closed

fdietze force-pushed the scala-dom-types branch 2 times, most recently from 9b2ddf9 to 5d8ecc6 Compare November 8, 2017 13:51

cornerman force-pushed the scala-dom-types branch 3 times, most recently from 033142a to 50dd451 Compare November 9, 2017 23:08

cornerman commented Nov 10, 2017

View reviewed changes

mariusmuja force-pushed the scala-dom-types branch from 59cb269 to d7c39cf Compare November 24, 2017 06:45

cornerman commented Nov 24, 2017

View reviewed changes

mariusmuja reviewed Nov 24, 2017

View reviewed changes

cornerman commented Nov 24, 2017

View reviewed changes

raquo mentioned this pull request Nov 24, 2017

add class alias for className raquo/scala-dom-types#14

Closed

cornerman commented Nov 25, 2017

View reviewed changes

cornerman and others added 14 commits November 25, 2017 15:23

Use scala-dom-types

555cf24

fixes outwatch#20, outwatch#49

expose DomWindowEvents and DomDocumentEvents for observable dom events

9e281e6

fixes outwatch#89

handle boolean properties correctly

32d52d3

Fix for boolean props

3c27f25

separate encoding for values of property and attribute

24297a7

Fix compilation error on 2.11. Ignore initEvent warnings

4f1e526

not stringify property values, can have type any

6966323

rename DomTypes without Dom-prefix

1423ade

Some renames, simplified attribute encoder

3f5b482

fix boolean attribute encoder

cf91cf0

fix custom prop function

de5edb2

more compatability attributes

11bd82a

Added some backwards compatibility attributes/tag

802ca56

update domtypes

6d7833d

cornerman force-pushed the scala-dom-types branch from ca3f327 to 6d7833d Compare November 25, 2017 14:24

make for not deprecated

abf7ff5

cornerman merged commit 5c2a968 into outwatch:master Nov 25, 2017

fdietze deleted the scala-dom-types branch November 25, 2017 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use scala-dom-types #62

Use scala-dom-types #62

cornerman commented Oct 15, 2017

raquo commented Oct 16, 2017 •

edited

raquo commented Oct 16, 2017

cornerman commented Oct 16, 2017

cornerman commented Oct 16, 2017 •

edited

LukaJCB commented Oct 16, 2017

cornerman commented Oct 16, 2017

raquo commented Oct 17, 2017 •

edited

cornerman commented Oct 17, 2017 •

edited

raquo commented Oct 21, 2017

fdietze commented Oct 22, 2017

cornerman commented Oct 24, 2017

raquo commented Oct 24, 2017

raquo commented Oct 24, 2017

cornerman commented Oct 25, 2017

fdietze commented Nov 8, 2017

cornerman Nov 10, 2017

cornerman Nov 10, 2017

cornerman Nov 10, 2017

cornerman commented Nov 24, 2017

cornerman Nov 24, 2017 •

edited

mariusmuja Nov 24, 2017

LukaJCB commented Nov 24, 2017

mariusmuja Nov 24, 2017

cornerman Nov 24, 2017

mariusmuja Nov 24, 2017

cornerman Nov 24, 2017

mariusmuja Nov 24, 2017

cornerman Nov 24, 2017

mariusmuja Nov 24, 2017 •

edited

mariusmuja commented Nov 24, 2017

cornerman Nov 25, 2017

cornerman Nov 26, 2017

cornerman commented Nov 25, 2017

fdietze commented Nov 26, 2017

Use scala-dom-types #62

Use scala-dom-types #62

Conversation

cornerman commented Oct 15, 2017

raquo commented Oct 16, 2017 • edited

raquo commented Oct 16, 2017

cornerman commented Oct 16, 2017

cornerman commented Oct 16, 2017 • edited

LukaJCB commented Oct 16, 2017

cornerman commented Oct 16, 2017

raquo commented Oct 17, 2017 • edited

cornerman commented Oct 17, 2017 • edited

raquo commented Oct 21, 2017

fdietze commented Oct 22, 2017

cornerman commented Oct 24, 2017

raquo commented Oct 24, 2017

raquo commented Oct 24, 2017

cornerman commented Oct 25, 2017

fdietze commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cornerman commented Nov 24, 2017

cornerman Nov 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmuja Nov 24, 2017 • edited

Choose a reason for hiding this comment

mariusmuja commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cornerman commented Nov 25, 2017

fdietze commented Nov 26, 2017

raquo commented Oct 16, 2017 •

edited

cornerman commented Oct 16, 2017 •

edited

raquo commented Oct 17, 2017 •

edited

cornerman commented Oct 17, 2017 •

edited

cornerman Nov 24, 2017 •

edited

mariusmuja Nov 24, 2017 •

edited