WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 112 forks source link

placeholder causes unnecessary DOM updates #113

Closed asapach closed 7 years ago

asapach commented 7 years ago

Adding a placeholder, as described in the docs seems to cause unnecessary DOM updates: https://codepen.io/asapach/pen/aLzWdq?editors=0010

var html = hyperHTML.bind(document.body);
var content = {
  any: hyperHTML.wire(document, ':passed')`<b>passed</b>`,
  placeholder: hyperHTML.wire(document, ':failed')`<i>failed</i>`
};

setInterval(() => {
  html`test ${content}`;
}, 500);

Using promises, as in the example, has no effect:

var promise = Promise.resolve(hyperHTML.wire(document, ':passed')`<b>passed</b>`)
var content = {
  any: promise,
  placeholder: hyperHTML.wire(document, ':failed')`<i>failed</i>`
};

Getting rid of placeholder fixes it.

ematipico commented 7 years ago

How do you test that placeholder causes more renders than needed?

asapach commented 7 years ago

Open dev tools and see the elements flicker: image

WebReflection commented 7 years ago

a synchronous placeholder makes no sense. You use placeholder when you want to show something while the resolution is being resolved. The resolution must be asynchronous so this is expected, and documented as such.

WebReflection commented 7 years ago

To clarify:

Every intent can be setup asynchronously by simply using the placeholder property.

You use the placeholder if you have an asynchronous value, otherwise there's no reason to use a placeholder.

A resolved promise is still asynchronous so that would always work.

asapach commented 7 years ago

@WebReflection, see my note about the promise above. Is it expected that it will still use the placeholder if the promise is resolved? How about performance?

asapach commented 7 years ago

To expand on performance: it inserts the placeholder and then immediately replaces it with the resolved content on every render. Which causes the whole dom tree to be invalidated.

WebReflection commented 7 years ago

that's how promises work. You pass them around. If these are resolved, you still pass them around.

const anyAsyncValue = Promise.resolve(2);

console.log(1);
anyAsyncValue.then(console.log);
console.log(3);
// 1
// 3
// 2

How about performance?

You have an asynchronous value, whenever it's resolved, it's resolved. Promises are unobservable so I am not sure what do you expect from this library. You use placeholder when you have an async value. You cannot know if the value is resolved or not. If the value is not async, you don't use a placeholder.

To expand on performance: it inserts the placeholder and then immediately replaces it with the resolved content on every render. Which causes the whole dom tree to be invalidated.

it's not immediately, it's never immediatly with Promises, it's when the async value is resolved which will not cause the whole dom tree to be invalidated, I've no idea where you read that.

There is no solution so it's not too productive to keep talking about resolved promises. The placehoder is a utility, use if if you need it, avoid it otherwise.

This was a non existent issue because you are not using an asynchronous value.

There are already proven cases of asynchronous values and all of them work without a single issue.

asapach commented 7 years ago

Here's a more realistic example that caused me to open this issue: https://codepen.io/asapach/pen/ZXGmoM?editors=0010 Imagine that the resolved promise is actually a fecth() call.

Try clicking the button multiple times. I'm seeing that that on every render the nodes are removed and re-inserted into the DOM. Am I not using it correctly? What is the correct usage then and how is it different from those in the docs?

If there's no solution, I would discourage using placeholder altogether, because it leads to performance problems.

WebReflection commented 7 years ago

If you resolve the promise a part from the logic you are in charge of re-rendering once it's fullfilled. If you use a placeholder, you fetch directly in there so that you use case would once again be non existent.

Placeholder is a utility, if it doesn't solve your problem, don't use it.

This will never be fixed vacause promises are non observable.

You can use also existing DOM nodes as placeholder .

Placeholder will stay because it already proved its utility.

please move on, thank you