diff-match-patch
diff-match-patch copied to clipboard
Port diff_match_patch into Typescript
Ported the Javascript version of diff_match_patch into Typescript.
- [x] I ported the JavaScript tests. All tests passing.
- [x] I checked in the generated Typings declaration (dts)
- [x] I also checked in the compiled UMD binary for those wanting to directly import that into their projects.
To run tests:
cd typescript/tests/
tsc
node ./built/tests/diff_match_patch_test.js
node ./built/tests/speedtest.js
How well do the types line up with @types/diff-match-patch
to ease migration?
They are mostly a match with the exception of Diff. I decided to match other language conversions here and make Diff a class. It’s neater that way and allows us to call the fields operation and text rather than use them by their indices. This would make it harder to migrate tho for those that had been using @types and the JS version.
What are your thoughts Kyle?
I'm not going to be migrating from the @types
so it's not personally a blocker. From my purview, it seems ok to force a migration since the typescript here is still bundled in a big repo (no one can install this directly). However, it's reasonable to reach out to @karak, @pspeter3, and @vsiao to see what they think as prior authors of the old definitions.
I'm more worried about the breaking change to the JavaScript than the TypeScript. Having real guaranteed correct types (because the compiler generates the .d.ts) is worth the migration pain.
Type definition in @types is just a mirror. I agree with the library itself has one and it's officially preferred. I think the issue is the change in JavaScript after all.
One thing we can do to help ease migration is support both types. [number, string] and the Diff class.
Here's a quick prototype of my suggestion.
tl;dr.
- Methods that accept a diff, use IDiff instead (which is a union type of [number,string] and the Diff class)
- For folks that have been using new Diff(number,string) but have been referencing [0] and [1], we can use getters and setters to wrap operation and text in the Diff class.
--- Edit As I think about this more, I don't think the above would work as you could end up having defined a Diff as an array. [1, 'text'] and error trying to access its operation field.
I think it makes more sense to just provide helpers for anyone coming from the JS world.
If you've defined your Diffs as arrays, perhaps we could defined a factory method on Diff .fromArray()
and if you're accessing a diff object's operation via the index or via operation, the above should getters / setters should handle that. Like so
Hey. Wasn't this issue closed, or unmerged yet?
Still unmerged. Things move slow here. You can tell because it says "Open" and not "Closed" or "Merged" 😉
It'd be nice if this were easier to review, but if it's up-to-date and all the tests pass it could be worth merging for no other reason than to keep it moving. Things I know I would like to see with this:
- Incorporation of some kind of fix for the split surrogate pair issue in
toDelta
. See #69 #80 - Some discussion about whether this work could also eliminate the role that ClosureCompiler plays, or if we have to stick with that for now.
- Updated docs on the role
tsc
plays and how to build the project. Currently those are on the Wiki which is admittedly hard to track in source control. - Deletion of the old
_uncompressed
JS.