rework on DOM(html,svg) tags #56

chandu0101 · 2017-01-08T08:40:12Z

currently its 60k LOC(inline +noinline) (generated) , each tag defined like ..

     
 @inline
 def caption(
  tabIndex: U[Int] = undefined,
is: U[String] = undefined,
classID: U[String] = undefined,
contentEditable: U[String] = undefined,
role: U[String] = undefined,
style: U[js.Any] = undefined,
hidden: U[Boolean] = undefined,
ref: U[(_ <: dom.html.Element) => _] = undefined,
key: U[String | Int] = undefined,
dir: U[String] = undefined,
id: U[String] = undefined,
.. many more
  @exclude extraAttributes: U[js.Object] = undefined)(children: ReactNode*) : ReactElement  = {
      val props = FunctionMacro()
    if(extraAttributes.isDefined && extraAttributes != null) addJsObjects(props,extraAttributes.get)
    if (developmentMode) React.createElement("caption",props,children :_*)
    else inlineReactElement("caption",props,children :_*)
 }

In above code we used macro to create js.Object from method params which expands to bunch of updateDynamic calls and we have extrraAttributes for unknown props at compile time which will add extra execution time while combing objects ,i think with new @ScalaJSDefined trait changes came in scala.js 0.6.14 we can make it better!

New Proposal :

lets have a global trait with all dom attributes

@ScalaJSDefined
trait DOMProps extends js.Object {
var  tabIndex: U[Int] = undefined,
 var is: U[String] = undefined,
var classID: U[String] = undefined,
var contentEditable: U[String] = undefined,
var role: U[String] = undefined,
var style: U[js.Any] = undefined,
......
}

def caption(props:DOMProps)(children:ReactNode*) = React.createElement("caption",props,children:_*)

//call site 
caption(new DOMProps { id = "hello" ; tabIndex = 2})(children)

Pros :
1)we can have type safe DOM tags under 500LOC instead of 30K LOC before
2)no need of macros
3)no extra run time cost

Cons :
1)we need some extra typing new DOM Props{ at call site , but i think its not a big deal

The text was updated successfully, but these errors were encountered:

nafg · 2017-01-08T08:51:36Z

we need some extra typing new DOM Props{ at call site , but i think its not a big deal

I think it is a big deal. At the end of the day (1) we have to write a lot of markup, and (2) it's important that (with some squinting) it has a strong resemblance to the HTML it represents

1)we can have type safe DOM tags under 500LOC instead of 30K LOC before

Isn't it code-generated?

2)no need of macros

I think they're justified

3)no extra run time cost

No reason why macros can't achieve that

What would be really neat is if we could make a general-purpose macro or codegen library that lets you turn any trait into a typesafe name-arguments method. Then it would have wider appeal and the maintenance burden could be spread more widely. DOM would just be a special case of it.

nafg · 2017-01-08T08:59:56Z

Also I don't like the idea of `vars

chandu0101 · 2017-01-08T09:38:41Z

I think it is a big deal. At the end of the day (1) we have to write a lot of markup, and (2) it's important that (with some squinting) it has a strong resemblance to the HTML it represents

makes sense :) , i am convinced to ditch my New Proposal .

dispalt · 2017-01-08T18:51:21Z

Isn't it code-generated?

That doesn't mean there is 0 cost, that increases size of the js which is a problem for me. I should make a small case to generate real numbers, Ill try to do that.

This has much less code and I like the flow. https://github.com/dispalt/sri-vdom @nafg take a look.

What would be really neat is if we could make a general-purpose macro or codegen library that lets you turn any trait into a typesafe name-arguments method. Then it would have wider appeal and the maintenance burden could be spread more widely. DOM would just be a special case of it.

https://gist.github.com/dispalt/9c7449bfdf4bc2d41c31248b722daf68

I will probably just stick to using my own sri-web component for now, if this doesn't change.

chandu0101 · 2017-01-08T20:46:06Z

That doesn't mean there is 0 cost, that increases size of the js which is a problem for me. I should make a small case to generate real numbers, Ill try to do that.

you mean production code ?

simple test

@ScalaJSDefined
trait DOMProps extends js.Object {
  var id: js.UndefOr[String] = js.undefined
  var key: js.UndefOr[String] = js.undefined
  var tabIndex: U[Int] = undefined
  var is: U[String] = undefined
  var classID: U[String] = undefined
  var contentEditable: U[String] = undefined
  var role: U[String] = undefined
  var style: U[js.Any] = undefined
  var hidden: U[Boolean] = undefined
  var ref: U[js.Function1[(_ <: dom.html.Element),_]] = undefined
  var dir: U[String] = undefined
  var className: js.UndefOr[String] = js.undefined
}

  @inline
  def div1(props: DOMProps) = React.createElement("div", props)

  @inline
  def div2(id: js.UndefOr[String] = js.undefined,
           key: js.UndefOr[String] = js.undefined,
           tabIndex: U[Int] = undefined,
           is: U[String] = undefined,
           classID: U[String] = undefined,
           contentEditable: U[String] = undefined,
           role: U[String] = undefined,
           style: U[js.Any] = undefined,
           hidden: U[Boolean] = undefined,
           ref: U[(_ <: dom.html.Element) => _] = undefined,
           dir: U[String] = undefined,
           className: js.UndefOr[String] = js.undefined) = {
    val p = js.Dynamic.literal()
    id.foreach(v => p.updateDynamic("id")(v))
    key.foreach(v => p.updateDynamic("key")(v))
    className.foreach(v => p.updateDynamic("className")(v))
    tabIndex.foreach(v => p.updateDynamic("tabIndex")(v))
    is.foreach(v => p.updateDynamic("is")(v))
    classID.foreach(v => p.updateDynamic("classID")(v))
    contentEditable.foreach(v => p.updateDynamic("contentEditable")(v))
    role.foreach(v => p.updateDynamic("role")(v))
    style.foreach(v => p.updateDynamic("style")(v))
    dir.foreach(v => p.updateDynamic("dir")(v))
    ref.foreach(v => p.updateDynamic("ref")(v))
    React.createElement("div", p)
  }

test1 : 

 val div1_v = div1(new DOMProps {
      key = "div1_key";
      tabIndex = 4;
    })
    dom.window.console.log(div1_v)
//fullOpt code 
var a=new k.Object;a.key="div1_key";a.tabIndex=4;var a=ua.createElement("div",a)

 val div2_v = div2(key = "div2_key", tabIndex = 4)
    dom.window.console.log(div2_v)

//fullOptCode 
a=E.createElement("div",{key:"div2_key",tabIndex:4});

test2 : 

  val div1_v = div1(new DOMProps {
      key = "div1_key";
      style = js.Dynamic.literal(padding = 10);
      ref = js.defined((e :dom.html.Div) => println(e))
    })
    dom.window.console.log(div1_v)
//fullOptCode 
var a=new h.Object;a.key="div1_key";a.style={padding:10};a.ref=function(a){Ma(Na().C.y,a+"\n")};a=Fa.createElement("div",a);Ga().console.log(a);

 val div2_v = div2(key = "div2_key", style = js.Dynamic.literal(padding = 10),ref = (e :dom.html.Div) => println(e))
    dom.window.console.log(div2_v)

//fullOptCode 
var a={padding:10},b=Oa(function(a){Ma(Na().C.y,a+"\n")}),c={key:"div2_key"};void 0!==a&&(c.style=a);void 0!==b&&(c.ref=function(a){return function(b){return(0,a.I)(b)}}(b));a=Fa.createElement("div",c);Ga().console.log(a)


test3 : 

val div1_v = div1(new DOMProps {
      key = "div1_key";
      style = js.Dynamic.literal(padding = 10);
      className = "sfdf";
      role = "dgsdgf";
      dir = "sadsad"
    })
 dom.window.console.log(div1_v)

//fulOpt
var a=new k.Object;a.key="div1_key";a.style={padding:10};a.className="sfdf";a.role="dgsdgf";a.dir="sadsad";a=ta.createElement("div",a);

val div2_v = div2(key = "div2_key", style = js.Dynamic.literal(padding = 10), className = "sdad", role = "asdas", dir = "sadsad")
    dom.window.console.log(div2_v)

//fullOpt
var a={padding:10},b={key:"div2_key",className:"sdad",role:"asdas"};void 0!==a&&(b.style=a);b.dir="sadsad";a=ta.createElement("div",b);

with current version there is an inconsistent behaviour if we pass a callback/object .

Current fullOpt.js file size is = 10KB

Now i just added your version of vdom and imported import com.dispalt.vdom.prefix_<^._

fullOpt.js file size is = 38KB

val div3_v = <.div(^.key := "div3_key",^.className := "div3_class")
dom.window.console.log(div3_v)
//fullOptCode
var a=ie.Yb,b=(he(),ke()).jc;he();var c=K().Ra,d=new J,g=new le;g.S=b.S;g.n="div3_key";g.Fc=c;b=(he(),ke());0===(512&b.ya.p)&&0===(512&b.ya.p)&&(me||(me=(new ne).a()),b.na=me,c=b.ya,b.ya=(new N).da(c.q,512|c.p));var b=(he(),K().Ra),c=new oe,k=new pe;k.Tb="div3_class";

Now file Size = 61KB
to be honest i have no idea of whats going on there.

Test Project i used : https://github.com/chandu0101/scalatest-error

I will probably just stick to using my own sri-web component for now, if this doesn't change.

as i said earlier vdom will come in separate project, and we will also provide your version of vdom so that people can choose what ever they want. your version is an easy selling point for people coming from scalajs-react ;)

dispalt · 2017-01-08T20:58:40Z

So sorry I wasn't clear. The current version, aka 60k sloc, has a real cost. Hence I used my version.

However, with your new approach I might change over. So my viewpoint is, it's better than the current approach. If you don't take the new approach, I'll stick with my version. My only complaint would be lack of immutability, although more verbose is less error prone.

It's hard to test because as you use more tags and thus less code gets eliminated, the combinatorial explosion of the current version will take up more space. Moving to the Dom version saves code, I just don't know the full extent, because practically it will differ alot from code base to code base.

chandu0101 · 2017-01-08T21:09:25Z

The current version, aka 60k sloc, has a real cost. Hence I used my version.

can you please elaborate on real cost ? maintenance or compile times or ..
may we will just remove noinline version , i don't think a valid use case for that. then again we still have 30LOC of dev.
@nafg whats u r take on inconsistent behaviour when we pass callback/object attributes with current approach ? we have extra checks 0!==a&&(b.style=a) and increase in code generated(very little though). New Proposal suffers from none of the above!

because practically it will differ alot from code base to code base.

indeed , but sri version of vdom will beat scalajs-react vdom interms of performance ,code size(output) and type safety any day! :p

dispalt · 2017-01-08T21:31:18Z

can you please elaborate on real cost ? maintenance or compile times or ..

code size.

I am supporting your proposed new approach, but I am still hesitant on the mutable-ness.

chandu0101 · 2017-01-08T21:46:43Z

but I am still hesitant on the mutable-ness.

well we can provide two variants ;)

chandu0101 · 2017-01-08T22:57:25Z

regarding inconsistency with object/function attributes scala-js/scala-js#2714

and now current version output code is small and more performant!(http://stackoverflow.com/a/21436082/986387) than New Proposal , may we should stick with existing one and use our own OptionParam instead of js.UndefOr ?

nafg · 2017-01-08T23:31:04Z

On Sun, Jan 8, 2017, 4:09 PM Chandra Sekhar Kode ***@***.***> wrote: The current version, aka 60k sloc, has a real cost. Hence I used my version. can you please elaborate on real cost ? maintenance or compile times or .. may we will just remove noinline version , i don't think a valid use case for that. then again we still have 30LOC of dev. @nafg <https://github.com/nafg> whats u r take on in consistent behaviour when we pass callback/object attributes with current approach ? we have extra checks 0!==a&&(b.style=a) and increase in code generated(very little though).

I did not understand that. What? New Proposal suffers from none of the above!

Any reason the macro can't be changed to output whatever you want users to write without a macro? because practically it will differ alot from code base to code base.

…

indeed , but sri version of vdom will beat scalajs-react vdom interms of performance ,code size and type safety any day! :p — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGAUB1va9LoykYP1JvmgKwBmM_JAgNZks5rQVCGgaJpZM4LdpAh> .

chandu0101 · 2017-01-08T23:35:40Z

Any reason the macro can't be changed to output whatever you want users to
write without a macro

can you please show us an example how can we achieve this ? scala-js/scala-js#2714 i want cool if! section .

chandu0101 · 2017-01-08T23:47:14Z

here are my final thoughts :

Current Approach :

Pros :

Usability

Cons :

Large code base
Need macros magic
as we can't provide typesafety for data-* attributes ,it adds up execution time in creating object
Takes more time to publish/download package.
More time in compile/optimization ,with current HtmlTagsTest SvgTagsTest sbt "project test" test taking ~30 minutes of time.

New Proposal

Pros:

No magic , easy to define and maintain,easy to extend in user code base(for data-* attributes)
Less code (~500LOC <<<<<<<30KLOC) , easy to publish/download
Faster compile/optimization times!

Cons:

A little verbose at call site

chandu0101 · 2017-02-22T13:39:55Z

after playing with new approach , i have to admit that usability matters @nafg :) , to use View with style(which we need most of the times) View( new ViewProps { style = ..})() just killing me :( , I'll stick with current approach with few changes

We will use OptionalaParam type (inline version of js.UndefOr)
primitives comes with few fields defined at first and we will add new ones when user of our library needed them , this way we can avoid high compile/optimization times

import sri.macros.{
  FunctionObjectMacro,
  exclude,
  rename,
  OptDefault => NoValue,
  OptionalParam => U
}

  @inline
  def View(style: U[js.Any] = NoValue,
           onLayout: U[LayoutEvent => _] = NoValue,
           @exclude extraProps: U[ViewProps] = NoValue,
           @exclude key: String | Int = null,
           @exclude ref: ViewClass.type => Unit = null)(
      children: ReactNode*): ReactElement = {
    val props = FunctionObjectMacro()
    extraProps.foreach(v => { MergeJSObjects(props, v) })
    CreateElementJS(ViewClass, props, key, ref, children.toJSArray)
  }

for 99% of cases we don't need any other props , if user want to pass onMoveShouldSetResponder to View he/she can use extraProps or submit PR for that , I recommend sending a PR than using extraProps!. that being said i am not using dom primitives div,span,etc in my apps, I'll add id,className,style for all tags and some one please take care of adding other params used in your apps.

nafg · 2017-02-22T21:58:54Z

BTW you inline a lot, have you actually tested / benchmarked how much of a difference it makes in running time (and how it affects download time)?

…

On Wed, Feb 22, 2017 at 8:39 AM Chandra Sekhar Kode < ***@***.***> wrote: after playing with new approach , i have to admit that usability matters @nafg <https://github.com/nafg> :) , to use View with style View( new ViewProps { style = ..})() just killing me :( , I'll stick with current approach with few changes 1. We will use OptionalaParam type (inline version of js.UndefOr) 2. primitives comes with few fields defined at first and we will add new ones when user of our library needed them , this way we can avoid high compile/optimization times import sri.macros.{ FunctionObjectMacro, exclude, rename, OptDefault => NoValue, OptionalParam => U } @inline def View(style: U[js.Any] = NoValue, onLayout: U[LayoutEvent => _] = NoValue, @exclude extraProps: U[ViewProps] = NoValue, @exclude key: String | Int = null, @exclude ref: ViewClass.type => Unit = null)( children: ReactNode*): ReactElement = { val props = FunctionObjectMacro() extraProps.foreach(v => { MergeJSObjects(props, v) }) CreateElementJS(ViewClass, props, key, ref, children.toJSArray) } for 99% of cases we don't need any other props , if user want to pass onMoveShouldSetResponder to View he/she can use extraProps or submit PR for that , I recommend sending a PR than using extraProps! that being said i am not using dom primitives div,span,etc in my apps, I'll add id,className,style for all tags and some one please take care of adding other params used in your apps. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGAUE78K8DQTDmxCK4AWZ8F8faKd7oHks5rfDqrgaJpZM4LdpAh> .

chandu0101 · 2017-02-22T22:03:53Z

have you actually tested / benchmarked how much of a
difference it makes in running time ?

all primitive components should be inlined in my opinion! that being said no benchmark done with/without ..

and how it affects download time

i didn't understand this part,can you elaborate ...

nafg · 2017-02-22T22:20:56Z

On Wed, Feb 22, 2017 at 5:04 PM Chandra Sekhar Kode < ***@***.***> wrote: have you actually tested / benchmarked how much of a difference it makes in running time ? all primitive components should be inlined in my opinion! that being said no benchmark done with/without ..

Then I suggest you consider the tradeoffs that are facts, not opinions. For example how suppose I have a js.UndefOr and I need to convert it to your thingy?

and how it affects download time i didn't understand this part,can you elaborate ...

Because by inlining, the JS file is bigger, so it takes longer to load the page.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGAUG3CY-fmJ4rGT__A3v3DEw5mKoiBks5rfLDegaJpZM4LdpAh> .

chandu0101 · 2017-02-22T22:39:15Z

Because by inlining, the JS file is bigger, so it takes longer to load the
page.

View(style = myStyle)("Child") -> var p = {style : myStyle};React.createElement(ViewClass,p,"child"} , i think its not much code and when something changed react call re render most of the time we should optimize here, React suggests to remove React.createElement also facebook/react#3228

nafg mentioned this issue Jan 8, 2017

rework on third party components #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework on DOM(html,svg) tags #56

rework on DOM(html,svg) tags #56

chandu0101 commented Jan 8, 2017 •

edited

Loading

nafg commented Jan 8, 2017 •

edited

Loading

nafg commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

dispalt commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

dispalt commented Jan 8, 2017

chandu0101 commented Jan 8, 2017 •

edited

Loading

dispalt commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

nafg commented Jan 8, 2017 via email

chandu0101 commented Jan 8, 2017

chandu0101 commented Jan 8, 2017 •

edited

Loading

chandu0101 commented Feb 22, 2017 •

edited

Loading

nafg commented Feb 22, 2017 via email

chandu0101 commented Feb 22, 2017

nafg commented Feb 22, 2017 via email

chandu0101 commented Feb 22, 2017

rework on DOM(html,svg) tags #56

rework on DOM(html,svg) tags #56

Comments

chandu0101 commented Jan 8, 2017 • edited Loading

nafg commented Jan 8, 2017 • edited Loading

nafg commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

dispalt commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

dispalt commented Jan 8, 2017

chandu0101 commented Jan 8, 2017 • edited Loading

dispalt commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

chandu0101 commented Jan 8, 2017

nafg commented Jan 8, 2017 via email

chandu0101 commented Jan 8, 2017

chandu0101 commented Jan 8, 2017 • edited Loading

Current Approach :

New Proposal

chandu0101 commented Feb 22, 2017 • edited Loading

nafg commented Feb 22, 2017 via email

chandu0101 commented Feb 22, 2017

nafg commented Feb 22, 2017 via email

chandu0101 commented Feb 22, 2017

chandu0101 commented Jan 8, 2017 •

edited

Loading

nafg commented Jan 8, 2017 •

edited

Loading

chandu0101 commented Jan 8, 2017 •

edited

Loading

chandu0101 commented Jan 8, 2017 •

edited

Loading

chandu0101 commented Feb 22, 2017 •

edited

Loading