Migrate to typescript #170

s1na · 2019-01-03T13:29:31Z

This PR migrates the library to typescript. Part of the diff is only formatting, and due to running prettier on the codebase.

The PR needs further testing and is work in progress.

coveralls · 2019-01-03T13:34:19Z

Coverage decreased (-0.4%) to 99.248% when pulling 0c322f6 on typescript into fb864da on master.

src/index.ts

axic · 2019-01-03T14:19:37Z

src/index.ts

-  r = new BN(r)
-  s = new BN(s)
+  const rBN: BN = new BN(r)
+  const sBN: BN = new BN(s)


In generateAddress2 shadowing variables is not a problem. Any reason to make a distinction here?

I couldn't find how to shadow a variable with a different type without the typescript compiler complaining. In generateAddress2 the type stays the same.

If the type stays the same then there's no point to have that expression in the first place :)

Agree! but just tried removing the toBuffer expressions and it seems like generateAddress2 tests are passing parameters as string, and not Buffer contrary to docs. Added string type to its parameters (and docs). Let me know if you'd rather limit the input types to Buffer.

axic · 2019-01-03T14:20:57Z

src/index.ts

-exports.fromRpcSig = function (sig) {
-  sig = exports.toBuffer(sig)
+export const fromRpcSig = function(sig: string) {
+  const buf: Buffer = toBuffer(sig)


Same question here.

s1na · 2019-01-07T10:40:37Z

@krzkaczor This library uses documentation to generate docs. However documentation doesn't support typescript yet from what I've gathered (documentationjs/documentation#282).

Are you aware how the typescript community usually generate docs? Is typedoc a good alternative? I'm trying to use it in combination with typedoc-plugin-markdown, which for example generates such an output.

Update: Here's the result.

holgerd77 · 2019-01-07T11:18:22Z

@s1na Just stumbled above exactly the same on the common library transition. 😄 😄

Would also have chosen TypeDoc for the docs, have no deep insight though yet, maybe @krzkaczor can confirm if this is a solid choice.

krzkaczor · 2019-01-07T11:21:20Z

@holgerd77 @s1na getting to know docs generating tools for TS is on my todo list 😆 I heard good things about TypeDoc, so I guess it is a right choice!

s1na · 2019-01-07T11:27:41Z

@holgerd77 @krzkaczor 😄 Then I'll continue with typedoc for now. Let me know if you ever found a better alternative :)

s1na · 2019-01-07T14:00:26Z

@holgerd77 I'm trying to specify the type of functions which accept parameters or return values with multiple possible types, and I'm wondering what's the best strategy here.

It's possible to just use unions (Buffer | string) and keep the API as it is.
Break up functions to make it clear what will be the return type, given a parameter type. E.g. baToJSON accepts Buffer | Array and returns Array | string | null, which can be broken into baToJSON(Buffer): string and baToJSON(Array<?>): Array<?> (I'm not sure what specific array type it should support).
My personal preferred method would be to accept only a main type and not try to support every possible type, and then provide functions to easily convert between types. E.g. many of the functions accept any parameter and call toBuffer internally. They can all be modified to accept only Buffer, and have users of lib to make sure they convert to Buffer. This simplifies logic, and removes type dependent inconsistencies. This however entails introduces breaking API change.

I've seen a few such inconsistencies so far:

setLengthLeft accepts Buffer | Array and supposedly returns Buffer | Array, but I think it never actually returns Array.
unpad removes 0x from strings if they have it, but doesn't prepend it back after unpadding (as Buffer | Array don't need this).
generateAddress2 docs said it accepts only Buffer, but it was calling toBuffer and the tests were passing string.

holgerd77 · 2019-01-08T09:17:34Z

src/index.ts

+  | undefined
+  | BN
+  | BufferableArray
+  | Arrayable


Types and interfaces in the RLP implementation have gotten their own types.ts file. I'm not deep enough into TypeScript yet to judge what is better / best practice. So just asking: what is your reasoning here to do it this way?

I'm just used to defining types where they're relevant, but don't have a strong feeling about it, and if you'd prefer that I can move them to types.ts.

Alternatively, maybe it'd be possible to break the one big index.ts into couple of smaller files which contain their relevant types. E.g. buf.ts (all buffer related funcs), ecdsa.ts (priv/pub key, sign, ecrecover), hash.ts (keccak, sha256, ripemd160), addr.ts (generateAddress, etc.). It's difficult to categorize the functions nicely however 😄

Have no strong feeling about it, if you want just leave as is. Some reorg likely also makes sense on this library - see also e.g. #38. We should eventually do this in a second round though, think there is enough to concentrate on here.

holgerd77 · 2019-01-08T09:20:12Z

src/index.ts


 /**
 * [`secp256k1`](https://github.com/cryptocoinjs/secp256k1-node/)
 * @var {Object}
 */
-exports.secp256k1 = secp256k1
+export { secp256k1 }

 /**
 * Returns a buffer filled with 0s
 * @method zeros
 * @param {Number} bytes  the number of bytes the buffer should be
 * @return {Buffer}


Just as a note, maybe you just haven't gotten to that yet: types in doc comments have to be removed, see TSDoc standard.

Yeah, I still have to do that. I kept them around for now to help me with annotating the types :)

holgerd77 · 2019-01-08T09:29:41Z

@s1na Can you add some notes on TSDoc usage to the docs section of the EthereumJS docs, together with an introduced JavaScript/TypeScript sectioning like in Transpilation (a bit above) (you can state that this is now the "standard" unless someone else states something else 😄 😄 )?

holgerd77 · 2019-01-08T09:48:26Z

To your multi-type-specification question:

I am also very much in favor of your preferred approach and I think opinions stated in the past from e.g. @axic or @whymarrh also stated very much in that direction, so I think we should go on here and get the API (more) right and introduce breaking changes where necessary.

I think we should nevertheless do/discuss this on a case-by-case (method-by-method) basis, since the nature and usage setting of the different functions differ very much. So can you post here which method(s) you want to change to what new method signature and/or split-up functionality here so that we can get to a short adjustment/assurance before this is actually changed?

Maybe do one post with edits and a curated list of API changes as well as checkboxes if pushed? Then we don't loose oversight. Or otherwise around: I will directly start on the next comment with some suggestions, then we can take this as a starting point.

holgerd77 · 2019-01-08T10:02:36Z

Breaking API Changes

List with suggestions for breaking API changes

Moved here.

holgerd77 · 2019-01-08T10:15:22Z

Also put @vpulim in cc here for comments.

holgerd77 · 2019-01-08T10:44:47Z

Ok, I think I am through with suggestions on API changes, see above. I would especially love to hear some explicit statement from @axic if time permits, but also e.g. @krzkaczor being the TypeScript type expert and everyone else. This should be broadly discussed and confirmed/refined.

holgerd77 · 2019-01-08T10:45:23Z

@s1na What do you think?

s1na · 2019-01-08T14:05:48Z

@holgerd77 Great! Thanks for the detailed suggestions. I think they make sense. Just a few comments:

If I'm not mistaken, because typescript types are statically checked and not during runtime, HexString can only be implemented as a class, which means users must convert to/from for function invocations. Just writing this to be taken into consideration, I don't have a preference here.
For unpad we can also keep the same name but have three signatures.
If isPrecompiled is HF-dependent, maybe it can be moved to ethereumjs-common? (that is, if it has use-cases and we don't want to drop it).
There are still cases where they accept only Buffer, but still call toBuffer, e.g. bufferToInt. Should we keep them as they are, or rather throw an error if they receive an unexpected type? I believe this helps with discovering bugs.

holgerd77 · 2019-01-08T14:30:38Z

@s1na Numbered your stuff for easier reference.

Ah, yes, that doesn't make sense to require users to convert. Then we should at least pass input params to a dedicated check method and throw if no hex-prefix provided, and always return prefixed hex strings. Does this make more sense? Or any other options (anyone?)?
Ok, that's more convenient and makes more sense.
Yes, might make sense - at least the right place there - this would need some additional structure in the JSON files. Not sure if it's worth the effort, but we can keep this in mind (or directly open an issue).
I think these conversions should be removed. We should very much have a look that we keep track in the post above though, so that we document all that stuff and list all breaking API changes.

holgerd77 · 2019-01-08T14:31:06Z

@s1na Before you start nevertheless let's wait 1-2 days more for some more opinions on this.

axic · 2019-01-14T13:50:25Z

Ok, I think I am through with suggestions on API changes, see above. I would especially love to hear some explicit statement from @axic if time permits

I will need to properly read it, but I would prefer if it would be possible to make the agreed breaking changes in Javascript and make a last release with that, before merging the Typescript rewrite. This decouples the two changes and hopefully reduces the potential bugs/issues.

Does that makes sense?

holgerd77 · 2019-01-14T15:53:06Z

No, not really in my opinion. TypeScript is a perfect fit here to actually REDUCE the level of potential bugs since one is guided and safe-guarded by the type system. So would say we should stick with the combination of changes.

holgerd77 · 2019-01-14T15:55:30Z

Other opinions on that?

s1na · 2019-01-22T13:15:06Z

@holgerd77 would it maybe make the review process easier if for now set the ambiguous types to any, and do the breaking changes in a separate PR after this one?

holgerd77 · 2019-01-29T12:48:40Z

This also lacks displaying the main code changes in a renaming context, which makes it too hard to review and breaks file history. Can you see if you can do this two-step process here as well?

Also: I know this is a bit annoying and will probably cause an extra half-an-hour of work. But commit history got really messy here with all the changes and re-changes. Can you re-structure this into some clean commits and then do a force-push on this?

I think in this case it would be really worth the effort.

s1na · 2019-01-29T14:38:09Z

Yeah it had gotten messy, sorry about that 😅 I rewrote the commit history, hopefully it's more clean now.

This time I tried your suggestion and first moved index.js to src/index.js in one commit, then renamed it to src/index.ts in a second commit, and applied to modifications in a third commit. But github is still showing it as a deleted and newly added file.

However, this time I applied all of the changes to index in one commit. You can use its diff to review the actual transition of source code.

holgerd77 · 2019-01-30T11:31:15Z

@s1na All global constants from the index.ts file are now also added to the docs, e.g. Buffer.

Can you make sure (hopefully this is not a TSDoc bug?) that this is not the case and only the exported constants and functions are included?

package.json

s1na · 2019-02-01T10:49:28Z

Regarding documentation, added --excludeNotExported which fixes the issue you mentioned. Although I noticed there's another issue. The libraries we're re-exporting are not documented (BN, rlp and secp256k1). However I haven't yet found a work-around for this, there are a few issues on typedoc which are related to this, e.g. TypeStrong/typedoc#712.

holgerd77 · 2019-02-01T11:09:50Z

Hmm, seems we have to deal with this somehow for now since these are open issues on TypeDoc. I very much would like to prevent to manually post-edit the docs, this always went unreliable from past experiences.

Can we live with some addition to the README API section, something like:

Re-exports of other libraries:

BN (BN.js)
...

[Link to internal docs]

Together with the explicit library linking this might even be some sort of improvement.

If you're ok with it can you add? Thanks!

s1na · 2019-02-01T12:03:03Z

Yeah, that's probably the best approach for now. Added to README.md.

holgerd77 · 2019-02-04T13:46:49Z

Hi Sina, sorry, I merged #171 without thinking about the implications for the TypeScript branch. Can you rebase on top of this respectively integrate the changes? Hope this is not causing too much trouble... 😕

I will directly take on this for review after the update.

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

s1na · 2019-02-05T08:42:21Z

Hey, oh no not at all, it was a minor change :) rebased master.

holgerd77

Looks good! 😄 👍

holgerd77 · 2019-02-05T12:24:27Z

src/index.ts

+/**
+ * Keccak-256 hash of the RLP of null
+ */
+export const KECCAK256_RLP: Buffer = Buffer.from(KECCAK256_RLP_S, 'hex')


Checked for equivalency of the constants, ok.

holgerd77 · 2019-02-05T12:26:06Z

src/index.ts

+ * [`secp256k1`](https://github.com/cryptocoinjs/secp256k1-node/)
+ */
+export { secp256k1 }
+


Did some short manual tests on re-export availability, ok.

holgerd77 · 2019-02-05T12:47:34Z

src/index.ts

+    }
+    return array
+  }
+}


Methods equivalent, ok.

holgerd77 · 2019-02-05T12:48:07Z

src/index.ts

+      throw new Error('invalid data')
+    }
+  }
+}


Did a line-by-line comparison of defineProperties, should be ok.

holgerd77 · 2019-02-05T12:52:52Z

Ok, have merged. Especially love this new documentation style, think particularly for this library TypeScript is a huge huge win also with the types available when coding and using the functionality. 🎊

@s1na I think a v6.1.0 release should do it here, what do you think?

s1na · 2019-02-05T12:56:55Z

Great! Yeah I think v6.1.0 should be okay, as no breaking change was introduced here.

s1na added PR state: WIP PR state: do-not-merge type: refactor labels Jan 3, 2019

axic reviewed Jan 3, 2019

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

axic reviewed Jan 3, 2019

View reviewed changes

holgerd77 reviewed Jan 8, 2019

View reviewed changes

This was referenced Jan 10, 2019

Auto-generated documentation with TSDocs ethereumjs/ethereumjs-account#30

Closed

New configuration package for TypeDoc documentation ethereumjs/ethereumjs-config#11

Open

Breaking API changes (stricter type requirements) #172

Closed

s1na removed PR state: WIP PR state: do-not-merge labels Jan 28, 2019

s1na force-pushed the typescript branch from 85f91e7 to 852222b Compare January 29, 2019 14:32

holgerd77 reviewed Jan 30, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

holgerd77 reviewed Jan 30, 2019

View reviewed changes

package.json Show resolved Hide resolved

s1na force-pushed the typescript branch from 852222b to 9e84303 Compare February 1, 2019 10:43

s1na added 6 commits February 5, 2019 08:56

Add tsc, tslint and prettier config files

1b03b86

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Mv index.js to src/

0ca5751

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Rename src/index.js to src/index.ts

50f507c

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Update package.json with new typescript configs

01b3779

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Convert index to typescript

a3d662b

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Update docs

0c322f6

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

s1na force-pushed the typescript branch from c43f8cf to 0c322f6 Compare February 5, 2019 08:27

holgerd77 approved these changes Feb 5, 2019

View reviewed changes

holgerd77 merged commit 153b7c3 into master Feb 5, 2019

holgerd77 deleted the typescript branch February 5, 2019 12:49

s1na mentioned this pull request Feb 21, 2019

Structure related functions in separate source files #182

Merged

holgerd77 mentioned this pull request Mar 20, 2019

Auto-generated documentation with TSDocs ethereumjs/ethereumjs-blockchain#95

Closed

snyk-bot mentioned this pull request Nov 25, 2019

[Snyk] Upgrade ethereumjs-util from 5.2.0 to 6.2.0 ajesse11x/Civil#12

Open

cgewecke mentioned this pull request Apr 23, 2020

Enforce hex prefixing for address strings #241

Merged

Migrate to typescript #170

Migrate to typescript #170

Conversation

s1na commented Jan 3, 2019

coveralls commented Jan 3, 2019 • edited

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

s1na commented Jan 7, 2019 • edited

holgerd77 commented Jan 7, 2019

krzkaczor commented Jan 7, 2019

s1na commented Jan 7, 2019

s1na commented Jan 7, 2019

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 commented Jan 8, 2019

holgerd77 commented Jan 8, 2019

holgerd77 commented Jan 8, 2019 • edited

Breaking API Changes

holgerd77 commented Jan 8, 2019

holgerd77 commented Jan 8, 2019

holgerd77 commented Jan 8, 2019

s1na commented Jan 8, 2019 • edited by holgerd77

holgerd77 commented Jan 8, 2019

holgerd77 commented Jan 8, 2019

axic commented Jan 14, 2019

holgerd77 commented Jan 14, 2019

holgerd77 commented Jan 14, 2019

s1na commented Jan 22, 2019

holgerd77 commented Jan 29, 2019

s1na commented Jan 29, 2019

holgerd77 commented Jan 30, 2019

s1na commented Feb 1, 2019

holgerd77 commented Feb 1, 2019

s1na commented Feb 1, 2019

holgerd77 commented Feb 4, 2019

s1na commented Feb 5, 2019

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

holgerd77 commented Feb 5, 2019

s1na commented Feb 5, 2019

coveralls commented Jan 3, 2019 •

edited

s1na commented Jan 7, 2019 •

edited

holgerd77 commented Jan 8, 2019 •

edited

s1na commented Jan 8, 2019 •

edited by holgerd77