[WIP] initial typescript conversion #60

rgbkrk · 2019-03-09T20:31:53Z

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 · 2019-03-09T21:00:34Z

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 · 2019-03-12T21:54:40Z

{} 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 · 2019-05-29T06:37:02Z

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

rgbkrk · 2019-05-29T06:37:49Z

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.


        initial typescript conversion


        run tsc over the source typescript

samelhusseini · 2019-07-27T07:32:15Z

@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 · 2019-07-27T07:37:40Z

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 · 2019-07-27T07:46:49Z

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 · 2019-07-29T15:30:51Z

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

NeilFraser and others added 2 commits Mar 9, 2019

initial typescript conversion

Loading status checks…

798e3e3

run tsc over the source typescript

Loading status checks…

230c9f6

rgbkrk force-pushed the rgbkrk:typescript branch from fc64826 to 230c9f6 May 29, 2019

Aug	SEP	Oct
	20
2019	2020	2021

google / diff-match-patch

[WIP] initial typescript conversion #60

[WIP] initial typescript conversion #60

rgbkrk commented Mar 9, 2019 •

edited

captainsafia commented Mar 9, 2019

NeilFraser commented Mar 12, 2019

rgbkrk commented May 29, 2019

rgbkrk commented May 29, 2019

samelhusseini commented Jul 27, 2019 •

edited

samelhusseini commented Jul 27, 2019 •

edited

samelhusseini commented Jul 27, 2019

rgbkrk commented Jul 29, 2019

google / diff-match-patch

Join GitHub today

[WIP] initial typescript conversion #60

[WIP] initial typescript conversion #60

Conversation

rgbkrk commented Mar 9, 2019 • edited

What works

Next steps

captainsafia commented Mar 9, 2019

NeilFraser commented Mar 12, 2019

rgbkrk commented May 29, 2019

rgbkrk commented May 29, 2019

samelhusseini commented Jul 27, 2019 • edited

samelhusseini commented Jul 27, 2019 • edited

samelhusseini commented Jul 27, 2019

rgbkrk commented Jul 29, 2019

rgbkrk commented Mar 9, 2019 •

edited

samelhusseini commented Jul 27, 2019 •

edited

samelhusseini commented Jul 27, 2019 •

edited