search

WebReflection / hyperHTML

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

Safari (Mac/iPad/iPhone): hyperHTML.wire() with static template does not guarantee the same generated element #338

Closed ikabirov closed 5 years ago

ikabirov commented 5 years ago

Tagged template function is not guaranteed to receive the same instance of a template array on Safari, even if it is used with the same template literal. It causes hyperHTML.wire() to generate a different instance of an HTMLElement.

Due to this issue it is not possible to make a component without adding wrapping element. Here is the simplest example reproducing this issue. https://codepen.io/anon/pen/QYwoRy Run it on Safari and wait for some time. On our devices it usually takes less than minute to see an alert.

It seems that Safari needs the _templateLiteral function fix similar to fix for Firefox<55.

WebReflection commented 5 years ago

I have removed all the unnecessary stuff to reproduce the issue, and I am going to file a bug to WebKit. https://codepen.io/WebReflection/pen/OdVrqN?editors=0010

This is the most annoying kind of bug ever, 'cause it's impossible to feature detect.

However, are you sure you want/need to retain any node reference at all? The fact nodes are preserved in hyperhtml shouldn't translate into "feel free to reference these and retain these around" because that's an implementation/performance detail you shouldn't care about.

In which scenario would this be an issue? I am just trying to understand if it's worth it to penalize Safari perf due random glitches on their GC.

WebReflection commented 5 years ago

Bug filed https://bugs.webkit.org/show_bug.cgi?id=193764

ikabirov commented 5 years ago

In which scenario would this be an issue?

I use it for creating component without top level wrapper. E.g now:

// button with internal state
import Button from 'button.js'

document.body.appendChild(Button({
  text: 'Save'
})

If result element can be changed, we must create wrapper for every component, and result DOM will be something like:

<body>
  <div>
    <button>Save</button>
  </div>
</body>

Assumption that element should not be changed allow us to remove unnecessary wrapper and receive something like:

<body>
  <button>Save</button>
</body>
WebReflection commented 5 years ago

could you bind a document fragment instead, as unique holder of the button?

const wm = new WeakMap;
const Button = identity => wm.get(identity) || set(identity);
function set(identity) {
  const fragment = document.createDocumentFragment();
  const render = hyperHTML.bind(fragment);
  wm.set(identity, render);
  return render`<button>${identity.text}</button>`;
}

or something similar ?

WebReflection commented 5 years ago

sorry, I know I haven't really provided a solution, just a ugly work around.

Let's see what Safari/WebKit folks say about their bug, so I'll figure out what I should do here.

ikabirov commented 5 years ago

Thank you! I'll try your work around.

ikabirov commented 5 years ago

could you bind a document fragment instead, as unique holder of the button?

it doesn't work, cause after document.body.appendChild(fragment) all button will be removed from fragment and moved to body. So after Button re-render it will be moved back to fragment and removed from body

WebReflection commented 5 years ago

it looks like they've closed the bug as duplicated, 'cause lit-html opened the same issue already.

Of course if it's Google complaining they listen and fix so ... good lit-html has the exact same issue.

I don't think I'll do anything for the time being, but I'll keep this open in case I change my mind.

ikabirov commented 5 years ago

May be add option to force use/ignore _templateLiteral function fix?

For safari ES6 it would be great to always turn it on (ES6 used in our debug environment) For production we use Closure Compiler with transpilation to ES5. it would be great to always turn it off (now property raw is enumerable, so we have unused overhead)

What about hyperHTML.forceTemplateLiteralFix?

WebReflection commented 5 years ago

the template literal fix is a third part dependency, nothing affected by random properties in this or that library.

https://github.com/ungap/template-literal#template-literal

WebReflection commented 5 years ago

So ... this would force Safari to the slow path, which I believe is the simplest/best option for the time being.

https://github.com/ungap/template-literal/pull/3

The ugly sniff/hack will be removed once they fix it in a future Safari version.

Your production code will still use the slow path, but:

  1. better safe than sorry
  2. iOS devices are already the fastest one out there, it won't hurt
  3. this might put more pressure in WebKit/Apple folks to fix this properly, since they care about being slowed down

Please try to use that patch in your env and tell me if everything is fine, thanks.

ikabirov commented 5 years ago

thank you

WebReflection commented 5 years ago

@ikabirov don't thank me yet, it's not planned to go out until you confirm it works as expected 😅

ikabirov commented 5 years ago

@WebReflection it seems everything is fine

WebReflection commented 5 years ago

So I've published it as 0.2.0 'cause even if it's a patch it changes slightly the behavior of the utility.

Whenever I'll find time to update all related projects, I will, meanwhile you can force-use that version.