WebReflection / domtagger

The hyperHTML's template literal parser
ISC License
43 stars 3 forks source link

Interpolated attributes broken for Edge and IE11 #20

Closed tiagomapmarques closed 4 years ago

tiagomapmarques commented 4 years ago

Interpolated attributes on lighterhtml are again broken for Edge and IE11. This is the same issue as #19 and this

Version tested: lighterhtml v1.4.1

WebReflection commented 4 years ago

do you mind trying v2 before I put my hands on this? also weird as domtagger is the latest and I haven't changed a bit in there in quite some time.

tiagomapmarques commented 4 years ago

I just noticed there was a v2. Doing it now :P

tiagomapmarques commented 4 years ago

v2 still has the same bug. on IE11 and Edge, the error is: "Unable to get property 'cloneNode' of undefined or null reference"

WebReflection commented 4 years ago

I need a super basic example of what's breaking. Also worth saying, I always suggest to have a single value per attribute, either ternary or not, as IE11 (and Edge) really has issues with attributes in general.

WebReflection commented 4 years ago

Actually, I think I still have the failing case locally .... will try to figure out why on earth this is failing, without me touching the domtagger for V2

WebReflection commented 4 years ago

So ... this is Edge, and it works ... what is this bug about?

Screenshot from 2019-11-15 08-32-08

WebReflection commented 4 years ago

... and this is IE11 ...

Screenshot from 2019-11-15 08-40-50

Accordingly, unless there is an example I can use to verify anything, there's no action for me to take, so I'll close this until a proper basic example that fails is shown.

WebReflection commented 4 years ago

code used to test:

var render = lighterhtml.render;
var html = lighterhtml.html;
function repeater(i) {
  const hasModifier = Math.random() < .5;
  render(document.body, html([
    '<div class="block__element',
    '">test</div>'
  ], hasModifier ? '--modifier' : ''));
}
var i = 0; setInterval(function () { repeater(++i); }, 1000);
tiagomapmarques commented 4 years ago

@WebReflection, you are correct. The example in the previous issue/PR is working.

However, I managed to find a simple example that shows a working vs non-working example. This is very weird and is making me believe this is indeed because of the order of the attributes. The examples are:

// Working interpolations
html`
  <div
    my-attr-two=${'hello'}
    my-attr-one="I like the number ${1} very much!"
  ></div>
`;
html`
  <div
    my-attr-two="hello"
    my-attr-one="I like the number ${1} very much!"
  ></div>
`;
html`
  <div
    my-attr-one="I like the number ${1} very much!"
    my-attr-two="hello"
  ></div>
`;

// Non-working interpolation
html`
  <div
    my-attr-one="I like the number ${1} very much!"
    my-attr-two=${'hello'}
  ></div>
`;

The error is definitely this: image

This error is caused in this line var fn = options.attribute(node, info.name, info.node); of the updates function - i.e. when an attribute is being inserted into the already correctly built document-fragment.

However, I do not know why this is happening. Can you give me some insights? :(

tiagomapmarques commented 4 years ago

@WebReflection bump

WebReflection commented 4 years ago

@tiagomapmarques I don't have much time these days to bloat the library for this awkward IE/Edge implementation, but also there's probably no solution whatsoever, as the template literal dictates the attribute order, and it's immutable.

As I've enabled this only recently, I suggest you stick with the previous rule: no interpolated attributes if you target IE/Edge, 'cause it's the best workaround I can think about right now.

Maybe I'll put a note in the README about this, or throw an error if the browser is IE/Edge and devs use interpolated attributes ... 'cause really ... there's honestly not much I can do, and I wouldn't add 1 extra KB of JS to deal with IE/Edge shenanigans.

I can't wait for Edge on Chromium to be out, so that everything will just work as expected.

WebReflection commented 4 years ago

Reopening so I don't forget to either put a note, throw an error, or find a fix that doesn't bloat the whole library.

tiagomapmarques commented 4 years ago

Just to be clear, the 1KB solution you're mentioning is my previous attempt at ordering the attributes right? If it is, then yeah, I unfortunately have to agree 😭

(But on the off-change it isn't, maybe it's worth a 5 min look? 😇here)

I can't wait for Edge on Chromium to be out, so that everything will just work as expected.

I'm keenly waiting for this day too :D

EDIT: the code linked above (just that commit) seems to definitely solves the issue on Edge and IE11 :D Will investigate a bit further if you think that code is small enough to be included. If not, no prob.

WebReflection commented 4 years ago

I would never bother other browsers with that, so it needs both tests and refactoring.

However, I really don't have time for this issue these days, so if it's urgent, use no interpolation.

I might have a look over the weekend, if I get bored in the train to Berlin

WebReflection commented 4 years ago

I hope this bug is now fixed forever. Thanks for the patience, and the fix hint 👍