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

Port diff_match_patch into Typescript #74

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samelhusseini
Copy link

@samelhusseini samelhusseini commented Jul 25, 2019

Ported the Javascript version of diff_match_patch into Typescript.

  • I ported the JavaScript tests. All tests passing.
  • I checked in the generated Typings declaration (dts)
  • 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

@rgbkrk
Copy link

rgbkrk commented Jul 29, 2019

How well do the types line up with @types/diff-match-patch to ease migration?

@samelhusseini
Copy link
Author

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?

@rgbkrk
Copy link

rgbkrk commented Jul 29, 2019

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.

@pspeter3
Copy link

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.

@karak
Copy link

karak commented Jul 30, 2019

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.

@samelhusseini
Copy link
Author

samelhusseini commented Jul 30, 2019

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

@karak
Copy link

karak commented Jul 18, 2021

Hey. Wasn't this issue closed, or unmerged yet?

@dmsnell
Copy link

dmsnell commented Jul 19, 2021

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 JS: Handle surrogate pairs correctly #69 Stop breaking surrogate pairs in toDelta()/fromDelta() #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.

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.

None yet

5 participants