davidjamesstone / superviews.js

Template engine targeting incremental-dom
http://davidjamesstone.github.io/superviews.js/playground/
246 stars 15 forks source link

elementPlaceholder is deprecated #10

Closed paulovieira closed 8 years ago

paulovieira commented 8 years ago

Incremental DOM v0.4.0 was released a few days ago: https://github.com/google/incremental-dom/blob/master/CHANGELOG.md

The main changes seem to be:

1) elementPlaceholder is now deprecated. More details: https://github.com/google/incremental-dom/pull/206

The skip function should be used instead: https://github.com/google/incremental-dom/blob/master/test/functional/skip.js

2) there's a new patchOuter function ("patches an Element rather than an Element's children")

It would be great if superviews could be updated to work with the new version.

davidjamesstone commented 8 years ago

Thanks Paul.

I'm away for a week but will get these changes done ASAP.

Thanks for the heads up.

Dave

davidjamesstone commented 8 years ago

HI Paul,

77762c6d8ab84bffac7e54e2066e7d8168df2d05

here's a commit for the skip implementation. It works like the if tag and attribute work. Any thoughts?

paulovieira commented 8 years ago

Cool, I'll try it out soon. I'll start a small project this week and I plan on using superviews instead of nunjucks (my usual choice for the template engine).

I'm also thinking about making a small grunt plugin and webpack loader. I'll send a pr with links for this stuff when they're done.

paulovieira commented 8 years ago

So, some thoughts on this.

With the update there is now 2 ways to use IDOM's skip() function from superviews. The following code is based on the example given in the readme.

1) using the <skip> tag:

<div>
    hello world
    <skip condition="ctx.something">
        I'm in a skip block. If the condition holds, idom will pass without touching me
    </skip>
</div>

This will compile to:

elementOpen("div")
  text("hello world")
  if (ctx.something) {
    skip()
  } else {
    text("I'm in a skip block. If the condition holds, idom will pass without touching me")
  }
elementClose("div")

However the compiled function is not valid. Incremental DOM will throw an error with the message:

Uncaught Error: skip() must come before any child declarations inside the current element.
    assertNoChildrenDeclaredYet @ assertions.js:162
    skip @ core.js:381
    ...

I think this way of using skip doesn't make much sense. It's like a different way of using the if attribute, no?

Having the skip functionality as a tag is confusing. The objective of skip (as I understand it) is to mark some element so that IDOM knows that it should be ignored when patching. Using an attribute makes much more sense.

2) using the skip attribute

<div>
    hello world again
    <span skip="ctx.something">
        I'm in a skip block. If the condition holds, idom will pass without touching me
    </span>
</div>

This will compile to

elementOpen("div")
  text("hello world again")
  if (ctx.something) {
    skip()
  } else {
    elementOpen("span")
      text("I'm in a skip block. If the condition holds, idom will pass without touching me")
    elementClose("span")
  }
elementClose("div")

This is pretty much the same code as in 1) and IDOM also throws the same error. So none of them is working.

But this is not how skip should be used anyway. The compiled template should instead be something like:

elementOpen("div")
  text("hello world again")
    elementOpen("span")
      skip()
    elementClose("span")
elementClose("div")

That is:

So in conclusion, the templates I used also don't make sense. The element where skip is going to be applied should be empty:

<div>
    hello world again
    <span skip>
    </span>
</div>

This is similar to elementPlaceholder (I think skip will replace it in future versions). But instead of using:

<skip title="I will render only once. Subsequent patches will be skipped." tag="span"></skip>

I think the templates would be much cleaner with a simple skip attribute:

<span title="I will render only once. Subsequent patches will be skipped." skip></span>

What do you think?

davidjamesstone commented 8 years ago

Wow - looks like I've really misunderstood this :)

Thanks for the great feedback.

Could I ask if you know of any documentation on this in IDOM? I sort of based the current implementation on a comment by sparhami.

davidjamesstone commented 8 years ago

Ok - I think I get it.

I've removed the tag as this makes no sense. skip attributes wrap the conditional around the children rather than the element itself.

Hope that has fixed it.

paulovieira commented 8 years ago

Sorry for the lack of reply. I have just checked and tested the compiled template and it working ok.

But I still think the skip attribute should be used as a boolean (like the checked attribute for checkboxes/radio).