Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some diff operations include actions that cancel each other out #90

Closed
bwindels opened this issue Jul 31, 2019 · 2 comments
Closed

Some diff operations include actions that cancel each other out #90

bwindels opened this issue Jul 31, 2019 · 2 comments

Comments

@bwindels
Copy link

With diff-dom 4.1.3, I'm diffing these two strings:

<div><em>foo</em> bar baz</div>
<div><em>foo</em> bar bay</div>

Using new DiffDOM().diff("<div><em>foo</em> bar baz</div>","<div><em>foo</em> bar bay</div>"), I get the following back:

[
  {
    "action": "modifyTextElement",
    "route": [
      1
    ],
    "oldValue": " bar baz",
    "newValue": " bar bay"
  },
  {
    "action": "removeTextElement",
    "route": [
      1
    ],
    "value": " bar bay"
  },
  {
    "action": "addTextElement",
    "route": [
      1
    ],
    "value": " bar bay"
  }
]

The first diff action is correct, but then the second and third cancel each other out and are redundant, and in my case even bothersome.

I'm using these, not to apply them to a DOM, but to visually mark the difference between two DOM trees (wrapping in <del> and <ins>), so having extra actions in there that shouldn't be shown messes it up a bit.

I've got a workaround, detecting exactly this case, but wanted to report as it's something potentially worth fixing.

@johanneswilm
Copy link
Member

Hey, this is interesting. I'll happily merge a fix.

@rzhornyk
Copy link

Hi @johanneswilm
Here is more interesting example of redundant changes. Swapping two DOM elements cause 200+ changes in text nodes.
https://jsfiddle.net/nqtrewb5/8/
Could you take a look?

rzhornyk added a commit to rzhornyk/diffDOM that referenced this issue Sep 28, 2020
clarkf added a commit to clarkf/matrix-react-sdk that referenced this issue Jan 29, 2023
Workaround is no longer necessary as of DiffDOM 4.2.1

See fiduswriter/diffDOM#90

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
t3chguy pushed a commit to matrix-org/matrix-react-sdk that referenced this issue Jan 31, 2023
* noImplicitAny fixes for MessageDiffUtils

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>

* Add tests for MessageDiffUtils

Adds mostly snapshot tests for MessageDiffUtils to guarantee consistent
behavior.

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>

* Strict mode fixes for MessageDiffUtils

Gets `MessageDiffUtils` to pass under `tsc --strict`.

Fixes element-hq/element-web#23665 - no longer errors,
though it still isn't correct.

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>

* Remove obsolete DiffDOM workaround

Workaround is no longer necessary as of DiffDOM 4.2.1

See fiduswriter/diffDOM#90

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>

---------

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants