Preact: Text node sometimes duplicated (after initial render) if its preceded by an empty node in IE9-11

[rmales]

This issue seems to have been introduced since 7.0.1. I guess this may be what is causing the tests to fail on Saucelabs (although I can't figure out what test it is from that output).

Example: http://jsfiddle.net/28ckgyzj/13/

[mkxml]

@rmales Yes, good catch. We are investigating this behavior.

[developit]

Ahh perfect, now we have an issue to track it under. Thanks 👍

[rmales]

If it helps at all reinstating the instanceof clause removed from this conditional seem to fix the bug (although I don't know if that affects the aim of the rest of the commit or anything else):
https://github.com/developit/preact/commit/c5553cefd67c4dc2ad262df2cfcb986114ef15a9#diff-a0896fa5d65b1d80ca4d496badbae50fL149

[developit]

Ahh - awesome! Thanks for the head start :)

[developit]

It's possible this is a Text node normalization issue in IE. I have a potential fix for that, will test it out ASAP.

[mkxml]

OK, Windows test setup working fine with preact. The problem seems to be related to __preactattr_ not always being correctly set in Text nodes on IE. Then it ends up using insertBefore creating the duplicated nodes because the node reference check doesn't match. Couldn't figure out where is the origin of this behavior yet. Guess I need to get familiar with the codebase :)

[developit]

Hmm. I wonder if Text nodes can't accept properties in IE?

[developit]

I seem to have found the fix. For some reason this bit was removed:

Without that check, IE throws when setting (or even accessing IIRC) .nodeValue.

Everything seems green now :)

[mkxml]

Humm, so in this patch you never reach the part of the code I was talking about. Seems fixed. Can you upload it to another branch so I can play with it in my local env?

[developit]

yup!

[developit]

Alright, it's up as bug/ie

[rmales]

I could be missing something, but having done a (quick) test that change doesn't seem to fix the issue in my example case (in IE11).

[developit]

@rmales how did you test the change?

[rmales]

@developit Manually in IE11 on Browserstack using my example code with a local version of Preact compiled (with npm run transpile:main) from your 'bug/ie' branch.

[developit]

can you try running with npm run build?

[mkxml]

@developit I think @rmales is right. The fix works to stop IE from throwing the nodeValue error, but the duplicated TextNode still appears and some unmount related tests still fail in IE11.

1 test was greened, but we still have some tests failing. All tests green on your env? (IE9?)

I can upload some screenshots from the karma reporter here if you want.

[rmales]

@developit Using npm run build doesn't seem to make a difference.

This is the output of failed tests I get from running the tests on Browserstack IE11 if that helps at all (although I'm not sure I have it configured correctly):
https://gist.github.com/rmales/cccaf39971757250b9ef2828412bd66d

I'm not sure if the tests actually hit the specific case in my example (and I haven't been able to figure out how to write an equivalent test). Weirdly if I toggle memory profiling on and off in the IE dev tools the (currently rendering) text node is duplicated again.

[mkxml]

Yes, I can confirm the duplicated text node. It seems flakey. Sometimes I get the expected behavior when debugging too. Something going on in the event loop?

[developit]

Ahhh! These are the few tests marked as "flakey"! The tests themselves were bailing in IE, but I couldn't reproduce the underlying issues, so I have them bypassed on SauceLabs (FLAKEY=false npm test). I think it's time to tackle them and get rid of that flag :)

[developit]

Random morning thought: maybe it's TextNode normalization kicking in?

[rmales]

@developit Text node normalisation sounds plausible, although I don't understand why it should be (periodically) occurring. Fiddling about, it seems like if you keep a reference to the text node the problem doesn't occur (in IE11).

Also, I guess https://github.com/developit/preact-portal/issues/2 may be the same bug.

[developit]

Ooooh good catch, yeah that looks like really similar symptoms. When you say "keep a reference" - what were you using to do that? I wonder if we could fix this by changing this line to be an intentional circular reference?:

// change from this:
dom[ATTR_KEY] = true;

// to this:
dom[ATTR_KEY] = dom;

... and then just be sure to delete that property on removal to avoid a leak.

[rmales]

@developit From my quick test it seems like a circular reference doesn't help. I was literally just keeping a global array of text nodes from the idiff function as a test.

[developit]

Another thing I'd wondered: maybe "filling" empty TextNodes with an zero-length space '\u200b' would work?

[mkxml]

In my tests the circular reference on dom[ATTR_KEY] = dom doesn't work.

It's probably working in @rmales' tests because keeping the reference with ATTR_KEY already set to true works.

The problem is that IE doesn't set custom properties in TextNodes reliably when appending them to the DOM, sometimes it reaches #L204 with props undefined because child[ATTR_KEY] === undefined.

It seems like a bug in IE we'll have to workaround.

I wonder if we explicitly need the ATTR_KEY in TextNodes, if we don't need it we can just go back to 6.4.0 behavior.

Test this forked bug/ie branch with the changes (bringing back some 6.4.0 code).

[developit]

Hmm - probably can't go back to 6.x behavior, that would break SSR again. We might be able to special-case TextNodes and have them always be included in child diffing, rather than using ATTR_KEY to flag them as being rendered by Preact. This was what I had originally done in the 7.0.x betas I think.

Basically L204 becomes:

else if (hydrating || props || child instanceof Text) {

[mkxml]

OK. I understand.

Check the following PR #462. After that bug/ie should be ready to merge to master and green the tests.

PS: Do we have tests simulating SSR/hydration today?

[developit]

I believe there are, have to check. Easiest simulation is to set scratch.innerHTML=renderToString(<Foo />) and then attempt to diff against it.

[tleunen]

@developit are the bugs with IE 11 fixed? The tests seem to pass.
I wonder if you have plan to release a new version with these fixes then :)

[developit]

Yes! I've been going back and forth on whether it'd be acceptable to publish the current changes from master as a 7.2 release while we're still nailing down the 8.0 breaking set of changes. I'd raised the question in the Preact Slack and people seemed inclined towards that approach.

[rmales]

@developit Do you have a timeline for a 7.2 release?

[developit]

I'll put up a beta right now.

[developit]

Released as 7.2.0 (beta).

[developit]

Is this still an issue? I think it was fixed in 7.2, which is now out of beta.

[rmales]

@developit I think its fixed, thanks.

[developit]

<3

[KrofDrakula]

Unfortunately, it still happens for me when using preact-portal (see issue #2). Not sure where the blame falls, though.

[developit]

I think we need to render a comment node for empty component roots, there's a PR open for it (#539) that is just missing a couple edge cases.