Migrate to TypeScript #92

whymarrh · 2019-03-07T21:49:31Z

This PR migrates the codebase to use TypeScript. This PR also replaces the Babel tooling with tooling from ethereumjs-config, resulting a project structure similar to the account, rlp, etc. libraries. I've made heavy use of any as a starting point for future restricting of the types (which will likely have to be accompanied by further refactoring). I've also dropped safe-buffer (as outlined in the organizations technical documentation).

coveralls · 2019-03-07T22:18:40Z

Coverage decreased (-2.06%) to 94.226% when pulling 7d54aea on whymarrh:migrate-ts into 2262597 on ethereumjs:master.

holgerd77 · 2019-03-07T23:04:43Z

👍

package.json

holgerd77 · 2019-03-07T23:10:22Z

Can you also add the return types to the functions?

holgerd77 · 2019-03-07T23:11:09Z

(corrected the comment above, please read on GitHub directly)

holgerd77 · 2019-03-07T23:14:32Z

Ok, that's REALLY a lot of any 😄, is it not possible to be more precise in many cases?

holgerd77 · 2019-03-07T23:15:46Z

(but if you think that's too much work load on this iteration - so be it - then we do it next round separately)

whymarrh · 2019-03-07T23:19:13Z

I am looking at being more specific with the types as I can, but there I think I might have to do a separate PR.

whymarrh · 2019-03-07T23:34:02Z

is it not possible to be more precise in many cases?

The main sticking point is that there are a good few functions in the index file that manipulate the arguments/overload the functions and to express that properly I would need to rewrite the functions themselves and I don't want them to get lost in the diff noise.

holgerd77 · 2019-03-08T08:54:25Z

Ah ok, no, them leave as-is, we can also add the return types later.

holgerd77 · 2019-03-08T08:56:16Z

.editorconfig

+insert_final_newline = true
+
+[*.md]
+indent_size = 4


What kind of editor configuration is this here? Even if this might be useful I would prefer not to side introduce here and eventually discuss along a separate PR, can you please remove?

Yup, I'll add this in a separate PR

holgerd77 · 2019-03-08T09:01:05Z

Generally: just realizing the scope of this work by scrolling completely through for the first time, really great! 😀 👍

holgerd77 · 2019-03-08T09:30:51Z

Can you do one integration test on this and open a PR on the VM pointing the blockchain dependency towards this branch so that we can see if CI runs through? This will largely help build up confidence that this has no side effects (especially after the experiences with the other TS transitions 😆).

whymarrh · 2019-03-08T15:11:07Z

Can you do one integration test on this and open a PR on the VM pointing the blockchain dependency towards this branch so that we can see if CI runs through?

For sure, I'll add that

whymarrh · 2019-03-11T19:39:22Z

Can you do one integration test on this and open a PR on the VM pointing the blockchain dependency towards this branch so that we can see if CI runs through?

Using this branch is a bit tricky with the new build process. The published versions will have compiled TS code where the branch doesn't. I could add a temporary step to ethereumjs/ethereumjs-monorepo#469 that compiles the source, how does that sound?

holgerd77 · 2019-03-11T21:06:24Z

@whymarrh Whatever is easier for you, you can also do a local test run and post the results here.

holgerd77 · 2019-03-11T21:59:33Z

This lacks a prettier.config.js file, see e.g. the one from the account library.

holgerd77 · 2019-03-11T22:35:01Z

Also .prettierignore.

holgerd77

Not completely through yet, some things to clarify or update, can already be addressed.

holgerd77 · 2019-03-10T10:13:55Z

src/index.ts

+const level = require("level-mem");
+const semaphore = require("semaphore");
+
+export default class Blockchain {


Just for notation: this will be the same kind of API breaking module loading as along the other TS transitions.

src/cache.ts

src/util.ts

src/dbManager.ts

package.json

whymarrh · 2019-03-12T02:53:34Z

I've got ethereumjs/ethereumjs-monorepo#469 to build successfully based on (a form of) these changes

holgerd77 · 2019-03-12T10:40:29Z

This has now linting errors since prettier is activated and CI is failing.

holgerd77 · 2019-03-12T13:10:14Z

src/index.ts

+  _initLock: any;
+  _putSemaphore: any;
+  _staleHeadBlock: any;
+  _staleHeads: any;


Nice resorting and - generally - property collection, finally gives some overview what's inside. 😄 👍

holgerd77

To be continued... 😄

holgerd77 · 2019-03-12T13:12:45Z

src/index.ts

+      }
+      this._initLock.go();
+    });
+  }


constructor(): Ok.

holgerd77 · 2019-03-12T13:14:07Z

src/index.ts

+        }
+        cb();
+      }
+    );


get meta(), _init(): Ok.

holgerd77 · 2019-03-12T13:15:10Z

src/index.ts

+        self._headBlock = genesisHash;
+      }
+    }
+  }


getHeads(): Ok.

holgerd77 · 2019-03-12T13:17:10Z

src/index.ts

+      },
+      cb
+    );
+  }


Ok till here.

holgerd77 · 2019-03-12T13:20:34Z

src/index.ts

+        this._putBlockOrHeader(block, done, isGenesis);
+      }, cb);
+    });
+  }


lockUnlock extracted, good.

holgerd77 · 2019-03-12T13:24:57Z

src/index.ts

+        next();
+      }
+    }
+  }


_putBlockOrHeader(): Ok.

holgerd77 · 2019-03-12T13:28:14Z

src/index.ts

+    }
+
+    nextBlock(blockId);
+  }


Ok until here.

holgerd77 · 2019-03-12T13:29:25Z

src/index.ts

+   * Gets block details by its hash
+   * @deprecated
+   */
+  getDetails(_: string, cb: any) {


Is this hash -> _ replacement some convention to mark things as deprecated? Other reason?

The hash argument isn't used so it's a convention to note that it's not used

holgerd77 · 2019-03-12T13:30:12Z

src/index.ts

+
+  _saveHeads(cb: any) {
+    this._batchDbOps(this._saveHeadOps(), cb);
+  }


Ok till here.

holgerd77 · 2019-03-12T13:31:46Z

src/index.ts

+        cb();
+      }
+    });
+  }


Ok till here.

holgerd77

Ok, I'm through finally 😄. Apart from another prettier fix run and a handful minor comments to be addressed I think this should actually be ready to merge since tests are also passing on the VM. Really great job! 👍

holgerd77 · 2019-03-12T19:09:53Z

src/index.ts

+    });
+  }
+
+  _delBlock(blockHash: any, cb: any) {


Should be safe to use Buffer here as a type too as in delBlock().

I'll update this to use Buffer

src/index.ts

test/index.ts

whymarrh · 2019-03-15T14:47:44Z

@holgerd77 updated again, let me know how it looks now

holgerd77 · 2019-03-15T15:55:02Z

Hi @whymarrh, thanks for the updates, can you please have a final look at the CI run some time after you push and check that this is ok? Otherwise this is always blocking.

Linting is still failing, probably prettier fix run still needs to be applied.

holgerd77 · 2019-03-18T08:52:05Z

@whymarrh Can you do this final prettier run so that we can get this over the finish line?

whymarrh · 2019-03-19T03:17:00Z

Updated. We'll also want to drop 83fb9c0 from this PR or before we publish.

holgerd77 · 2019-03-19T06:15:26Z

@whymarrh Not completely getting it. Why did you introduce this in the first place and why didn't you remove now along this update? Is this something which needs further work?

whymarrh · 2019-03-19T10:20:52Z

@holgerd77 updated and removed 83fb9c0. Apologies for the confusion, what wasn't clear? To be able to use this package via Git in ethereumjs/ethereumjs-monorepo#469 the files entry needed to be dropped. I've dropped them now and we can deal with that PR separately now—that PR no longer works but that's fine, once we publish a version of this package I can update that PR with the version from the registry.

From that PR's description:

Note: while [ethereumjs/ethereumjs-blockchain#92] is in-progress this will be pointing to a Git URL of my fork. Once the PR is merged (and the tests here will serve as a useful smoke test) this PR will be updated with the correct version on the registry.

holgerd77

Ah sorry, wasn't aware of this connection.

Looks good now, will merge! 😀🎷🎺🥁

whymarrh force-pushed the migrate-ts branch from 569967d to 20d2957 Compare March 7, 2019 22:11

holgerd77 reviewed Mar 7, 2019

View reviewed changes

package.json Show resolved Hide resolved

whymarrh force-pushed the migrate-ts branch from 20d2957 to ccd0af5 Compare March 7, 2019 23:17

whymarrh force-pushed the migrate-ts branch 2 times, most recently from 7d54aea to ee24224 Compare March 7, 2019 23:32

whymarrh force-pushed the migrate-ts branch from ee24224 to 8052ee1 Compare March 7, 2019 23:48

holgerd77 reviewed Mar 8, 2019

View reviewed changes

whymarrh mentioned this pull request Mar 11, 2019

Update ethereumjs-blockchain to latest ethereumjs/ethereumjs-monorepo#469

Merged

holgerd77 suggested changes Mar 11, 2019

View reviewed changes

whymarrh force-pushed the migrate-ts branch 3 times, most recently from c3fb550 to 4373b4b Compare March 12, 2019 01:32

holgerd77 reviewed Mar 12, 2019

View reviewed changes

whymarrh force-pushed the migrate-ts branch 2 times, most recently from 417159c to bad4381 Compare March 15, 2019 14:47

whymarrh force-pushed the migrate-ts branch from bad4381 to 90e18de Compare March 19, 2019 03:15

whymarrh force-pushed the migrate-ts branch from 90e18de to 79e8047 Compare March 19, 2019 03:27

whymarrh force-pushed the migrate-ts branch from 79e8047 to 8301c82 Compare March 19, 2019 10:16

Migrate to TypeScript

8301c82

holgerd77 approved these changes Mar 19, 2019

View reviewed changes

holgerd77 merged commit a898360 into ethereumjs:master Mar 19, 2019

whymarrh deleted the migrate-ts branch March 19, 2019 10:37

Migrate to TypeScript #92

Migrate to TypeScript #92

Conversation

whymarrh commented Mar 7, 2019

coveralls commented Mar 7, 2019 • edited Loading

holgerd77 commented Mar 7, 2019

holgerd77 commented Mar 7, 2019 • edited Loading

holgerd77 commented Mar 7, 2019

holgerd77 commented Mar 7, 2019

holgerd77 commented Mar 7, 2019

whymarrh commented Mar 7, 2019

whymarrh commented Mar 7, 2019

holgerd77 commented Mar 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Mar 8, 2019

holgerd77 commented Mar 8, 2019

whymarrh commented Mar 8, 2019

whymarrh commented Mar 11, 2019

holgerd77 commented Mar 11, 2019

holgerd77 commented Mar 11, 2019

holgerd77 commented Mar 11, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whymarrh commented Mar 12, 2019

holgerd77 commented Mar 12, 2019

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whymarrh commented Mar 15, 2019

holgerd77 commented Mar 15, 2019

holgerd77 commented Mar 18, 2019

whymarrh commented Mar 19, 2019

holgerd77 commented Mar 19, 2019

whymarrh commented Mar 19, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

coveralls commented Mar 7, 2019 •

edited

Loading

holgerd77 commented Mar 7, 2019 •

edited

Loading