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

Use diff-match-patch for text diffs #1285

Merged
merged 2 commits into from
Feb 26, 2017

Conversation

lukechilds
Copy link
Contributor

@lukechilds lukechilds commented Feb 24, 2017

Resolves #1280

diff is really slow for large strings. This just swaps out the diff npm package for diff-match-patch which is waaaay quicker. Down from ~20mins to ~1.5s in the linked issue.

`diff` is really slow for large strings
@vadimdemedes
Copy link
Contributor

Thanks for submitting such an easy-win PR, love it! Obviously, I'm all for perf optimization, especially with such a little change. However, this module seems to be unmaintained. The last commit is from 2015, despite having open issues and PRs.

Perhaps you are aware of some alternatives?

Also, can we swap the usage of 'diff' to the other library, so that we don't have 2 dependencies that basically do the same thing?

@lukechilds
Copy link
Contributor Author

Ah, I chose that module as it was recommended in the issue: #1280 (comment)

Didn't realise it was unmaintained, I'll have a look for some others.

Also, can we swap the usage of 'diff' to the other library, so that we don't have 2 dependencies that basically do the same thing?

Only if it supports generating a patch. The diff lib was being used to generate a diff for strings and a patch for arrays/objects.

@vadimdemedes
Copy link
Contributor

Well, if there are no viable and just-as-fast diff libraries out there, we can go with this one at least for string diffs, I think. It's better to use this library, than to have 20m of waiting time.

@lukechilds
Copy link
Contributor Author

The only other I've found is: https://github.com/liddiard/text-diff

It's also pretty outdated and is just a wrapper around the same Google lib that the other project is a clone of: https://code.google.com/p/google-diff-match-patch/

@vadimdemedes
Copy link
Contributor

This one doesn't have tests, so it's not a good candidate. Ok, so I see there aren't that many diff libs out there, I think we may just go with the one you used in the PR. I will test how it works and what output it provides, thank you!

@lukechilds
Copy link
Contributor Author

No probs 👍

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukechilds thanks for the quick turnaround! Perhaps you could fix the merge conflict?

@lukechilds
Copy link
Contributor Author

@novemberborn Is it not already fixed in d402277?

@novemberborn
Copy link
Member

Oh interesting. My merge button was set to Rebase and merge, which can't work, but I can squash and merge.

@novemberborn novemberborn merged commit bd5ed60 into avajs:master Feb 26, 2017
@novemberborn
Copy link
Member

Thanks @lukechilds!

@lukechilds
Copy link
Contributor Author

I like to call that squirging. No problem 👍

@leoswing
Copy link

I like to call that squirging. No problem 👍

Does the google diff generate a patch? which it could be used for diff2html and produce a nice diff like git diff

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

Successfully merging this pull request may close these issues.

4 participants