Improve diff implementation

[jzaefferer]

Potential diff implementations:

• https://github.com/kpdecker/jsdiff
• https://github.com/cemerick/jsdifflib (Timo recommended this one, using it by monkey-patching already)

Related tickets: #335, #348, #478, #363, #483

[jzaefferer]

@leobalter now that we've got the modules split, we could just try to drop in jsdiff and see what happens.

[leobalter]

I'm thinking about how is the best way to bring jsdiff code, maybe as a npm dependecy and using its dist file in the build process or as a git submodule or anything else.

anyway, I'll work on this patch.

[leobalter]

@jzaefferer: I tried to use kpdecker/jsdiff without much success.

See: https://github.com/leobalter/qunit/tree/jsdiff

There are a lot of things that doesn't make me feel comfortable with this patch (and that's why I am not requesting a merge yet), I'll try to list some here.

1. The way QUnit is built from src files is unfriendly to add external dependencies, I tried something that doesn't appealed fine to me. I would like to first refator the src files to turn them all into modules that would pass through JSHint check by themselves. Doing this I would wrap JsDiff inside intro and outro src files, preventing it to noise the global scope.
2. Not all the main methods from JsDiff are covered by tests, and I don't feel they are enough as the proper method we would use is still untested: JsDiff.diffChars.
3. JsDiff uses a escape method that would conflict to our escapeText method and I don't feel it as totally reliable.

There are other arguments that I will still check if I am wrong or not but I would like to first try other solutions.

Btw, I would like to see a repository of the actual QUnit.diff we are using, maybe that should be kept by the jQuery foundation or even by @jeresig himself, as he is the author.

[leobalter]

I've started reading jsdifflib code to implement it and I got a question: is jQuery Foundation compatible with their kind of license?

Ref: https://github.com/cemerick/jsdifflib/blob/master/difflib.js#L1-L30

[leobalter]

I tried to implement jsdifflib on https://github.com/leobalter/qunit/tree/364-jsdifflib but is far from perfect yet.

[Krinkle]

The cemerick/jsdifflib I used in the VisualEditor test suite works, but has problematic rendering. Its css selectors conflict with QUnit. And because its output is inside the list item and table cell of QUnit, css rules of QUnit also wrongly apply to the table cells of the diff rendering. The result is a mess for anything other than short and simple values.

[jzaefferer]

Regarding the license: Its under BSD, which should be fine for us. @scottgonzalez can you confirm that?

[scottgonzalez]

We can probably include Simplified BSD since it's almost the same as MIT, but this uses Modified BSD which contains the no-endorsement clause. We'll need to talk to @joelgkinney about it.

[gauravmittal1995]

@jzaefferer @leobalter Hey, I want to work on this for GSoC 15. Can u help me get started??

[AnishChandran]

I would like to contribute ! Can someone get me started ?

[leobalter]

There some parts of the current diff tool that we would like to enhance. We can improve its code or use another external tool, if it doesn't bring more problems to deal with.

We can look at the following issues to see specific points to lead the way to improve it:

• Show diff only when it helps #335: Show diff only when it helps
• Confusing line breaks in object diff #348: Confusing line breaks
• Backslash in string inconsistency in diff #478: Backslash in string inconsistency
• Diff only leafs for (big) objects #363: Diff only leafs for (big) objects
• QUnit.diff should be given dumped values before html escape #483: QUnit.diff should be given dumped values before html escape

[AnishChandran]

I thought of starting with #335 It´s a great idea to use the length factor to determine whether the diff is valid or not ! Changing the head.js to check the length condition at first would help this ryt ?

[jzaefferer]

It´s a great idea to use the length factor to determine whether the diff is valid or not !

As I wrote in #335, comparing the length isn't as easy as it seems.

Changing the head.js to check the length condition at first would help this ryt ?

I'm not sure what head.js you're referring to.

Anyway, I think in this case it would be good to start with a few diff test cases that show the potential for improvements. A group of actual/expected values that currently end up with more or less useless diffs.

[supunasp]

Gsoc 2015 - QUnit - Better diff output

I am interested in this and I am trying to understand the code any help would be great

[gauravmittal1995]

@leobalter @jzaefferer Hey, How does the following look?

This is using google's diff-patch-match with some additional changes done by me. Currently its supported for strings, but we can add it for array objects as well.

[leobalter]

The diff should work not only for strings, but for dumped object representations. See our QUnit.dump to check how it shows arrays, functions, objects, etc.

I can't say how good the diff looks from 2 strings only, it needs more information.

[leobalter]

@asped08, if you write a failing assertion, (like assert.equal({ foo: "foo" }, { foo: "bar" })) you can see the current diff in QUnit's test report. From there, there are some fixes and improvements on the already addressed issues.

[AnishChandran]

I just thought that we could deal with #348 first since it seems to be little less complicated
http://fiddle.jshell.net/sj6kdj9g/
Is this an simple example about confusing line breaks in diff ? since the "gre": "foo" should be aligned with "foo": "bar ? should we focus on qunit.js and qunit.css and change the way in which the diffs are displayed ?

[shivamdixit]

Just wanted to know that what are the issues in using jsdifflib directly? If we directly import it as a module.

[garan]

Just a heads-up.
I thought I would mention prettydiff here, to make sure you guys have considered it.

I have been using jsdifflib, as it looked to be the best option, months ago (not sure why I skipped prettydiff ..probably seemed like too much at the time).
However, I only use difflib.SequenceMatcher() and found jsdifflib code easy to adapt.

Prettydiff was originally developed from jsdifflib and has been developed quite a bit further.

The only feature of prettydiff that I'm interested in is character-diffs.
However, if you are displaying on the front-end, you might want to check it out.

Cheers.

P.S. These 'diff' modules really should keep their diff code separate, ideally in a separate module. I'm tempted to fork..

[jzaefferer]

On the list above, only #363 is still open, and we have a WIP PR for that. I don't think we need this meta ticket anymore.

[mr21]

@garan, I recently finish a diff implementation here: https://github.com/Mr21/diff.js
It's propose only a window.diff() who takes two arrays and that's it.

So if you want to make a diff between two strings you can do:

result = diff(
    "first_string".split(""),
    "second_string".split("")
);