Don't replace text nodes if there's nothing to replace

[ellatrix]

If the callback returns false, no text nodes should be replaced.
This is particularly a problem if you parse with a MutationObserver. The change is detected, and twemoji parses again, creating an infinite loop.

[WebReflection]

uhm ... can you please tell me more about this problem? with a real piece od DOM I can test against? Thank you

[WebReflection]

P.S. asking because IIRC there is a test already covering the return false case, thanks for any further info

[ellatrix]

If all callbacks for all emoji in a text node return false, the text node is replaced nevertheless. This is a problem if you use a MutationObserver to parse any new text nodes. It creates an infinite loop. The observer detects a change => twemoji parses, detects emoji but does not replace them with images (callbacks return false) => twemoji replaces the node anyway with a new fragment => the observer detects a change. And it starts all over. :) In any case, twemoji should not replace the node if it doesn't have to.

[WebReflection]

OK, makes sense ... but ... MutationObserver works asynchronously, the test change you made does not test this or make you changes safe against this API. I think tests shold be updated and consistent with the problem, just dropping few checks and adding more synchronous when the problem is an asynchronous infinite loop does not feel right to me.

Moreover, you changed the twitter.js file which means you haven't read much about how to contribute or change. So here the catch: twitter.js is automatically generated, you change that even on your host and you are doomed 'casue that's not what changes here.

twemogi-generator.js is eventually the file, it's on the top comment of your twitter.js file.

I will find some time to avoid the problem you mentioned, eventually improving tests to test against such problem too.

Feel free to be faster than me or please stay tuned and wait here, since I'll come back and close this once it's done.

Cheers

[WebReflection]

P.S. also your img check is wrong since that's inside the loop and won't solve a thing wih multiple scheduled changes, you are assuming the last img = null is the only one that matters.

[ellatrix]

Yes, it's in a loop, but it will never be replaced if src is false, so I believe this works fine. If there are one or more changes it will go through. Sorry about patching the wrong file, saw over that.
Not sure if more checks are needed. The problem is that twemoji needlessly replaces nodes with the same content. The problem with MutationObserver is a result of that.

[WebReflection]

I understand, but line 636 of resulting twemoji.js clearly checks for src to not be false.

I am not saying this problem you are facing is not real, I am saying this pull request is not that good because it address thw wrong file, it patches in a not so good way, and it does not provide tests against the API that should support.

I am sure you understand what I am saying, this project is widely used, I don't want to easily accept PR or rush solutions. I see the problem, this PR does not realy solve it. Still wrapping my head around this gotcha.

Cheers

[WebReflection]

OK, I've changed the test that was indeed checking that nodes were changed anyway, and I've introduced a simple boolean flag that is set to true only when an image is created. This is not optimal, in terms of computation, but it should fix your problem.

Can you please confirm latest push solves it? Thank you!

[ellatrix]

Yep, does the same thing and works. Thanks!

[WebReflection]

It doesn't technically does the same thing, it actually fixed a bug I am glad nobody spotted before.

If you look close, the img = assignment you were relying on, assumes that as soon as one image is assigned, any other part of the text field should too.

Meaning as soon as one single emoji was found, the while ((match = re.exec(text))) loop would have appended the same img node over and over in the very same line.

My change fixes that, and it resets the img value as null whenever it happens, plus it ensure in a proper boolean way there was for sure a change to consider for the parsed node.

I know how you feel about your PR, but now that I've explained why that check wasn't enough I hope things are better for you and whoever uses this library.

Thank you

[ellatrix]

Hm, there are still nodes replaced that don't need to be replaced.
Shouldn't there be modified = false; appended to the first while block?

[ellatrix]

P.S. You were right about checking img, my mistake. :)

[WebReflection]

Shouldn't there be modified = false; appended to the first while block?

Not sure it should ... the replacement happens per fragment and the fragment is used only if at least one node has changes ... what kind of scenario are you running twemoji on?

You should not have infinite loops anymore, you might have triggered changes but these should vanish in the next iteraction.

[WebReflection]

never mind ... you are right, it should be reset each while loop ...

[WebReflection]

.... right ... how's 1.3.1 doing? Sorry for that, I explained why img chec was wrong and replicated through my own variable ... seppuku !!!

[ellatrix]

Perfect, thanks a lot! :)

[WebReflection]

\o/