Skip to content
This repository has been archived by the owner on Aug 5, 2024. It is now read-only.

[WIP] initial typescript conversion #60

Closed
wants to merge 2 commits into from
Closed

Conversation

rgbkrk
Copy link

@rgbkrk rgbkrk commented Mar 9, 2019

Following on from #58, I've kicked off the conversion of diff-match-patch starting with @NeilFraser's starting point as the first commit. I've included the built copy for now, though I may strip that out and rebase it based on feedback.

One interesting thing between Neil's built version and my built version:

diff --git a/typescript/built/diff_match_patch.js b/typescript/built/diff_match_patch.js
index 8fbb030..dafa48a 100644
--- a/typescript/built/diff_match_patch.js
+++ b/typescript/built/diff_match_patch.js
@@ -387,7 +387,7 @@ var diff_match_patch = /** @class */ (function () {
      */
     diff_match_patch.prototype.diff_linesToChars_ = function (text1, text2) {
         var lineArray = []; // e.g. lineArray[4] == 'Hello\n'
-        var lineHash = {}; // e.g. lineHash['Hello\n'] == 4
+        var lineHash = Object.create(null); // e.g. lineHash['Hello\n'] == 4
         // '\x00' is a valid character, but various debuggers don't like it.
         // So we'll insert a junk entry to avoid generating a null character.
         lineArray[0] = '';

Not quite sure why typescript goes that route to make an object with a null prototype (that still behaves like an object).

What works

  • Base tests ✅
  • Speed tests ❌
    • The speed tests use diff_match_patch_uncompressed.js which is not generated (by name) here
  • Typing! ✅

Next steps

  • Determine how well the types line up with that of @types/diff-match-patch to ease migration
  • Make speed tests run
  • Determine how the uncompressed and compressed versions will be stored (I need direction here)

@captainsafia
Copy link

From my understanding, using “{}” to create a dictionary means if inherits the properties of the Object prototype, whereas using Object.create with a null parameter does not inherit any of the properties.

I looked up some more stuff about it and learned that maps created this way have faster access times in some cases. Interesting.

https://stackoverflow.com/questions/34480709/why-is-object-create-so-much-slower-than-a-constructor

By the way, cool work!

@NeilFraser
Copy link
Contributor

{} vs Object.create(null) is an interesting one. Performance considerations aside, Object.create(null) is prefered when using an object as a map with arbitrary keys, since one of those keys might be 'toString' or one of the other six properties inherited from Object.prototype. However, DMP was written before Object.create(null) existed. Thus it uses the more dangerous {}.

There are two places where {} is used as a map. One place is in match_alphabet_. This one is harmless since the keys are always single characters. At this time, there are no properties on Object.prototype with single character names. The other place is diff_linesToChars_. This one is dangerous. As such there's a hasOwnProperty check before each access so that we don't read in inherited properties. However, hasOwnProperty was only added in ES3, so Netscape 4 and early Safari versions don't support it. That's why we test for the existence of the hasOwnProperty function before calling it.

These days of course this level of backwards compatibility is completely ludicrous.

@rgbkrk
Copy link
Author

rgbkrk commented May 29, 2019

I just went on paternity leave for the last few months, I'm jumping back into this now. 😄

@rgbkrk
Copy link
Author

rgbkrk commented May 29, 2019

Looked at the git diff since I last touched this, seems I can safely rebase and go forward as the changes were to a header file and the dart code.

@samelhusseini
Copy link

samelhusseini commented Jul 27, 2019

@rgbkrk Typescript doesn't do anything special here.
You TS file just has Object.create(null) instead of {} hence why the two JS built files are different:
see https://github.com/google/diff-match-patch/pull/60/files#diff-1a73c059bb6730ae237f4a4e3497e1aaR449

@samelhusseini
Copy link

samelhusseini commented Jul 27, 2019

Ah I think I understand the source of confusion here. The JS built file in Neil's zip is outdated. Tr building it and you should get the same result.

@samelhusseini
Copy link

Also I worked on a conversion not realizing this PR existed, if you'd like to take a look, I'd love to hear your thoughts on: #74

One design decision I want to point out is making Diff a class with text and operation properties. On the one hand that's more consistent with other language conversions and is cleaner to define and access than using a Tuple (Array), but I'm wondering if that's worth it if it makes migration more tedious.

@rgbkrk
Copy link
Author

rgbkrk commented Jul 29, 2019

@samelhusseini hey no problem, I've ended up being swamped with other work so I'm glad you've picked this up.

@rgbkrk
Copy link
Author

rgbkrk commented Aug 23, 2021

Closing in favor of #74

@rgbkrk rgbkrk closed this Aug 23, 2021
@rgbkrk rgbkrk deleted the typescript branch August 23, 2021 21:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants