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

API messed up 1.0.0 -> 1.0.3 #14

Open
bebbi opened this issue Sep 3, 2018 · 3 comments
Open

API messed up 1.0.0 -> 1.0.3 #14

bebbi opened this issue Sep 3, 2018 · 3 comments

Comments

@bebbi
Copy link

bebbi commented Sep 3, 2018

1.0.0 worked.
In 1.0.3, the API is somehow messed up. The test harness does not detect it.

// that's what it should be but is not:
var a = [[DIFF_EQUAL, 'ab'], [DIFF_INSERT, '123'], [DIFF_EQUAL, 'c']]
var b = dmp.diff_main('abc', 'ab123c', false)
// Instead it's a strange object
console.log(b)
[ { '0': 0, '1': 'ab' },
  { '0': 1, '1': '123' },
  { '0': 0, '1': 'c' } ]
console.log(Array.isArray(a[0])) // true
console.log(Array.isArray(b[0])) // false
console.log(b)
assertEquivalent(a, b); // yes, equivalent

And it breaks ES6 clients:

const [eq, str] = a[0]  // works
const [eq, str] = b[0]  // breaks
@JackuB
Copy link
Owner

JackuB commented Sep 3, 2018

Yea, the change happened here https://github.com/JackuB/diff-match-patch/pull/13/files#diff-168726dbe96b3ce427e7fedce31bb0bcR76

It's emulating array for backward compatibility. But the destructing use case is not taken care of.

  1. We could add Symbol.iterator to the Diff object (this would require some kind of transpilation)
  2. Publish this as breaking and go for v2 (and release 1.0.4 without this change)
  3. Patch it to use array (current tests are passing)
diff_match_patch.Diff = function(op, text) {
  return [op, text];
};

Wondering if there is any reason they did that - most likely to keep the API 1:1 with rest of the implementations. Will try to look into it a bit more.

@bebbi
Copy link
Author

bebbi commented Sep 4, 2018

I was going to submit an issue in google but saw yours , thanks.
If google is happy with status quo, I'd propose option 2 as the one that can break in least possible ways, although it's a bit a strange api. Lets see what they say..

JackuB added a commit that referenced this issue Sep 4, 2018
@JackuB
Copy link
Owner

JackuB commented Sep 4, 2018

Released 1.0.4 with a rollback https://github.com/JackuB/diff-match-patch/releases/tag/1.0.4
Will hold on with releasing v2 as there is a chance they will rollback the API change

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

2 participants