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 to typescript #51

Open
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@dryajov
Copy link

commented May 19, 2019

TL;DR

Suggested tsconfig.json changes

Here are a set of suggested changes to tsconfig which hopefully decrease interop friction and increase Typescript and JS ergonomics.

(Please look at the comments for explanation)

{
  "extends": "@ethereumjs/config-tsc",
  "include": ["src/**/*.ts", "test/**/*.ts"],
  "compilerOptions": {
    "baseUrl": ".",

    "paths": {
      "*": ["definitions/*"] // enables stubbed type definitions to be placed/looked-up under this directory (can be anything, but I like `definitions` as the name)
    },

    "typeRoots": ["definitions", "node_modules/@types"], // loads _only_ the definitions located under these directories

    "declaration": true, // hint for tools (vscode) to generate missing type definitions

    "esModuleInterop": true, // enables default imports `import A from 'A'` instead of `import A = require('A')`

    "allowSyntheticDefaultImports": true, // should _not_ be needed if `esModuleInterop` is enabled, but it is explicitly disabled in the base config, so its explicitly enabled here for now

    "downlevelIteration": true // allows using `[...arg]` and `for of` if the target is `es5` or `es3`
  }
}

PR explanation

This is an initial port to typescript. Functionally, this module should behave exactly the same as the previous js implementation, however, backwards compatibility has been sacrificed in favor of better typescript and js ergonomics and stricter types where possible. See below for details.

Constant maps have been ported to Typescript enums

It is customary to define "enums" as const MyConsts = { MY_INVARIABLE_CONSTANT: 0x00 } in js, typescript provides a dedicated enum keyword and it is now possible to express the same as enum { MY_INVARIABLE_CONSTANT = 0x00 }, which is more idiomatic.

No export default and use of esModuleInterop for better CommonJS/Babel/etc interop

TL;DR

Use named exports instead of default export. Generally default exports is not what you need and it doesn't map well to CommonJS.


Typescript maps export default to module.exports = { default: {} } in CommonJS, which when called would end up being const {default: MyMod} = require('my-module') or const MyMod = require('my-module').default. This leads to breakage and confusion when consuming typescript modules with CommonJS systems.

Named exports on the other hand can be consistently reused across import and require:

CommonJs

const { MyExport } = require('my-module')

Import

import { MyExport } from 'my-module'

export = MyMod and import MyMod = require('my-module')

TL;DR

This is the usual way Typescript maps CommonJS modules to Typescript imports, but it requires a "special" import syntax, which gets confusing, and it's not immediately apparent unless you look into the module definitions, which import syntax to use.


From the Typescript handbook:

export = and import = require()

Both CommonJS and AMD generally have the concept of an exports object which contains all exports from a module.

They also support replacing the exports object with a custom single object. Default exports are meant to act as a replacement for this behavior; however, the two are incompatible. TypeScript supports export = to model the traditional CommonJS and AMD workflow.

The export = syntax specifies a single object that is exported from the module. This can be a class, interface, namespace, function, or enum.

When exporting a module using export =, TypeScript-specific import module = require("module") must be used to import the module.

In other words, this is intended to map directly to module.exports or exports, and that's great, except that you now have to figure out how a module should be imported - do you use import * as A from 'A', import {A} from 'A' or import A = require('A')? It gets confusing quickly.

To mitigate this, I strongly recommend using esModuleInterop, which helps interop with CommonJS and babel (and it also enables allowSyntheticDefaultImports, which is disabled in https://github.com/ethereumjs/ethereumjs-config/blob/master/packages/ethereumjs-config-tsc/tsconfig.json#L9). this allows consuming modules in a more natural way, e.g. import A from 'A' instead of import A = require('A'), and increase tooling interop.

For example, auto generate typings for CommonJS modules are usually defined in a style similar to:

export = A
declare class A {...}
namespace A {...}

With esModuleInterop you can now import it with import A from 'A' consistently without falling back to the typescript specific import A = require() syntax.

Recommendation to use "declaration" = true for auto generated definitions in tsconfig.json

Porting JS code to Typescript is usually relatively painless. DefinitelyTyped and the increased adoption of Typescript has reduced the amount of missing type definitions, but it still happens. For those cases, it is usually good to have some way of auto generating and loading type definitions. Vscode (and others), have tooling to generate stubbed type definitions, I strongly feel that using stubbed types is better than falling back to any. A stubbed type will detect when a missing method/property is being accessed, any will let anything thru. On top of that, stubbed types are easy to extend and eventually evolve into a full type definition.

@lgtm-com

This comment has been minimized.

Copy link

commented May 19, 2019

This pull request introduces 1 alert and fixes 1 when merging 0108de1 into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

@lgtm-com

This comment has been minimized.

Copy link

commented May 19, 2019

This pull request introduces 1 alert and fixes 1 when merging c596a84 into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

@holgerd77

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Mind blowing. That's totally awesome. 🤯🤗👍🎉

@holgerd77

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Well, again, mind blown, also how complete and well integrated this work already is, just had some first scroll through! Congrats! 😀

@holgerd77

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@dryajov Great to see that EthereumJS libraries can be used for the mustekala client project. 😄

@vpulim Do you eventually have some time to test this PR in the context of our client implementation and eventually also do some very rough benchmarking (just compare how far this goes on block height for 5 min or so)?

@dryajov

This comment has been minimized.

Copy link
Author

commented May 21, 2019

@holgerd77 Thanks for making/maintaining the libs 👍

I'm not sure if this will have any perf impacts at all, but its good to make sure either way 👍

@lgtm-com

This comment has been minimized.

Copy link

commented May 21, 2019

This pull request introduces 1 alert and fixes 1 when merging 6b0beb5 into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

@holgerd77

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Ok, did a first local test, linting and test commands work fine.

I also tested the peer communication example, works like a charm 👍, is node dist/examples/peer-communication.js the intended way to run it?

@holgerd77

This comment has been minimized.

Copy link
Member

commented May 27, 2019

npm pack doesn't work yet, these are the files added:

npm notice
npm notice 📦  ethereumjs-devp2p@2.5.1
npm notice === Tarball Contents ===
npm notice 2.8kB  package.json
npm notice 3.3kB  CHANGELOG.md
npm notice 12.0kB README.md
npm notice === Tarball Details ===
npm notice name:          ethereumjs-devp2p
npm notice version:       2.5.1
npm notice filename:      ethereumjs-devp2p-2.5.1.tgz
npm notice package size:  6.1 kB
npm notice unpacked size: 18.1 kB
npm notice shasum:        1479a87c43ad5955d9d5e65affeb0d3dfdc0a4c8
npm notice integrity:     sha512-QyN/li8TUNhab[...]Y8LCrhf8xV/GQ==
npm notice total files:   3
npm notice
ethereumjs-devp2p-2.5.1.tgz
@holgerd77

This comment has been minimized.

Copy link
Member

commented May 27, 2019

//cc @krzkaczor Could you eventually have a look at the suggestions to enhance the TypeScript configuration (see initial post)?

Show resolved Hide resolved definitions/chalk.d.ts Outdated
@krzkaczor

This comment has been minimized.

Copy link
Member

commented May 28, 2019

My few cents regarding tsconfig changes:

  • definitions this sounds like a good idea

  • "declaration": true, this is NOT needed as its part of a base config

  • downlevelIteration seems useful and probably should be part of a base config

  • esModuleInterop and allowSyntheticDefaultImports if we agree that we want to use them we should probably make it default and consistent across the org. I personally dont use these flags and I dont have a strong opinion either way. It would be nice if someone could present additional context here

@alcuadrado

This comment has been minimized.

Copy link
Member

commented May 28, 2019

  • downlevelIteration seems useful and probably should be part of a base config

I don't think this should be used unless we have an actual ES3/ES5 compilation target. @dryajov do you have one in mind?

@krzkaczor

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@alcuadrado i think, this org officially supports ES5 🤷‍♂

@alcuadrado

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@alcuadrado i think, this org officially supports ES5 🤷‍♂

Oh, if that's the case, I agree with you, that should be enabled in the base config.

@dryajov dryajov force-pushed the musteka-la:master branch from 6e94e8f to 89a56f2 May 28, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented May 28, 2019

This pull request introduces 1 alert and fixes 1 when merging 89a56f2 into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

@dryajov

This comment has been minimized.

Copy link
Author

commented May 28, 2019

Thanks for the feedback! @krzkaczor

My few cents regarding tsconfig changes:

  • definitions this sounds like a good idea
  • "declaration": true, this is NOT needed as its part of a base config
  • downlevelIteration seems useful and probably should be part of a base config

The idea is that this flags get adopted in the default config.

  • esModuleInterop and allowSyntheticDefaultImports if we agree that we want to use them we should probably make it default and consistent across the org. I personally dont use these flags and I dont have a strong opinion either way. It would be nice if someone could present additional context here

I've explained the reasoning behind it in the top PR comment under the export = MyMod and import MyMod = require('my-module') section. It's a bit lengthy so here is the gist:

Without this flag the export = syntax requires a typescript specific import syntax import A = require('a'), this allows using the more consistent import A from 'A' syntax.

This is important because a lot of npm module typings are written in the export = A manner, also tooling tends to generate stub typing in this way.

Please refer to the top PR comment for a much more in depth explanation.

@lgtm-com

This comment has been minimized.

Copy link

commented May 28, 2019

This pull request introduces 1 alert and fixes 1 when merging 3ccaf9f into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

@krzkaczor

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@dryajov thanks, makes sense!

@dryajov

This comment has been minimized.

Copy link
Author

commented May 29, 2019

Should we make these changes in the base config first and update this PR to reflect them, or should we just merge here without waiting for the base config changes and then do another pass once the base config has been updated and released?

@holgerd77

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Would have a tendency to first incorporate into the general config.

@dryajov

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@holgerd77 - sounds good. Should I go ahead and PR the base config then?

@holgerd77

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@dryajov yes, please, on the stuff where there was somewhat consensus about (don't have the complete picture). Just open a PR (without the release update, just the functionality) towards the ethereumjs-config repo.

@lgtm-com

This comment has been minimized.

Copy link

commented Jun 21, 2019

This pull request introduces 1 alert and fixes 1 when merging 4ceb60e into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

@holgerd77 holgerd77 referenced this pull request Jun 22, 2019

Open

Finishing up TypeScript transition work #52

0 of 7 tasks complete
@holgerd77
Copy link
Member

left a comment

Hi Dmitriy,
sorry for letting floating this around for so long, but this is some really extensive PR which need some concerted amount of review work.

I am nevertheless very dedicated to keep on this. I would say it will take realistically another 3-4 weeks, but if we both keep a bit on doing some updates it should be possible to get a release out by then (ah some update: I am no holidays for 2 weeks starting July 8th, so these will be eventually rather 5-6 weeks).

To get some overview I have compiled down a list with TODOs: #52
There are likely some still to be added along the way.

For this round I have done a first review concentrating on the structural changes and not touching the code base itself yet.

Let me know if you have got any questions or comment! 😄

Cheers
Holger

Show resolved Hide resolved .gitignore Outdated
Show resolved Hide resolved .travis.yml

- [pydevp2p](https://github.com/ethereum/pydevp2p) (Python)
- [Go Ethereum](https://github.com/ethereum/go-ethereum/tree/master/p2p) (Go)
- [Exthereum](https://github.com/exthereum/exth_crypto) (Elixir)

### Links

- [Blog article series](https://ocalog.com/post/10/) on implementing Ethereum protocol stack
- [Blog article series](https://ocalog.com/post/10/) on implementing Ethereum protocol stack

## License

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 22, 2019

Member

You don't have to do all our documentation work, but can you go roughly through the README and make this consistent again on a minimal level, so update links to point to the .ts sources and be workable again, update the example invocation and see that method signature is stuff is not necessarily reflecting the TypeScript stuff but is at least updated on eventual renamings and location changes?

This comment has been minimized.

Copy link
@dryajov

dryajov Jun 24, 2019

Author

I've updated the links to the correct files in the readme, but this particular link is dead - has the blog migrated somewhere else or should I just remove the link?

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

I have no idea, this one is pretty old, if you have the occasion just remove.

Show resolved Hide resolved definitions/rlp-encoding.d.ts
Show resolved Hide resolved examples/peer-communication-les.ts
Show resolved Hide resolved examples/simple.ts
],
"main": "./lib/index.js",
"main": "dist/src/index.js",

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 22, 2019

Member

This is not very beautiful and I would very much would like to avoid this having the src folder included in the distribution build. Was is difficult to set up a build structure with main/index.js as a target?

This comment has been minimized.

Copy link
@dryajov

dryajov Jun 24, 2019

Author

tsc replicates the project structure under dist by default, that's why the entry point is dist/src/index.js. Unless this is a really sticky point, I would leave it as is.

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

Hmm, this is different behavior from all our other transitions and I find it semantically wrong to have the src folder in the dist, so I would keep a bit on that. If this causes an unexpected amount of side work we can leave though.

Is it not possible to just set the baseUrl in the compiler options to the src folder or something?

Show resolved Hide resolved package.json
"allowSyntheticDefaultImports": true,
"downlevelIteration": true
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 22, 2019

Member

This needs an update as discussed. If this causes too much roundup work I would be also ok with having this adopted to the standard of discussion and left here for now. We could then integrate with the config library later on.

This comment has been minimized.

Copy link
@dryajov

dryajov Jun 24, 2019

Author

I'll update the corresponding build projects with this new settings.

This comment has been minimized.

Copy link
@dryajov

dryajov Jun 24, 2019

Author

Here is the corresponding PR - ethereumjs/ethereumjs-config#23

"allowSyntheticDefaultImports": true,
"downlevelIteration": true
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 22, 2019

Member

Same here.

@lgtm-com

This comment has been minimized.

Copy link

commented Jun 24, 2019

This pull request introduces 1 alert and fixes 1 when merging 3494cb7 into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional
@dryajov

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

npm pack doesn't work yet, these are the files added:

npm notice
npm notice 📦  ethereumjs-devp2p@2.5.1
npm notice === Tarball Contents ===
npm notice 2.8kB  package.json
npm notice 3.3kB  CHANGELOG.md
npm notice 12.0kB README.md
npm notice === Tarball Details ===
npm notice name:          ethereumjs-devp2p
npm notice version:       2.5.1
npm notice filename:      ethereumjs-devp2p-2.5.1.tgz
npm notice package size:  6.1 kB
npm notice unpacked size: 18.1 kB
npm notice shasum:        1479a87c43ad5955d9d5e65affeb0d3dfdc0a4c8
npm notice integrity:     sha512-QyN/li8TUNhab[...]Y8LCrhf8xV/GQ==
npm notice total files:   3
npm notice
ethereumjs-devp2p-2.5.1.tgz

Should be fixed now.

@lgtm-com

This comment has been minimized.

Copy link

commented Jun 24, 2019

This pull request introduces 1 alert and fixes 1 when merging ab79849 into 22c9a1c - view on LGTM.com

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional
@dryajov

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

@holgerd77 I've updated this PR with your comments, please let me know if there is anything else that needs to be addressed.

I've also added a PR to update the base config with the flags suggested in this PR. I'll update this PR as soon as the required flags are available in the base config.

@holgerd77

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

npm pack doesn't work yet, these are the files added:

npm notice
npm notice 📦  ethereumjs-devp2p@2.5.1
npm notice === Tarball Contents ===
npm notice 2.8kB  package.json
npm notice 3.3kB  CHANGELOG.md
npm notice 12.0kB README.md
npm notice === Tarball Details ===
npm notice name:          ethereumjs-devp2p
npm notice version:       2.5.1
npm notice filename:      ethereumjs-devp2p-2.5.1.tgz
npm notice package size:  6.1 kB
npm notice unpacked size: 18.1 kB
npm notice shasum:        1479a87c43ad5955d9d5e65affeb0d3dfdc0a4c8
npm notice integrity:     sha512-QyN/li8TUNhab[...]Y8LCrhf8xV/GQ==
npm notice total files:   3
npm notice
ethereumjs-devp2p-2.5.1.tgz

Should be fixed now.

Ah, this was actually just caused because I didn't build the project first hand, sorry.

If you find the occasion you could (a bit unrelated) change the "prepublish": "npm run build" script to "prepublishOnly": "npm run test && npm run build", which we have updated similarly in most of the other libraries.

@holgerd77

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

When I am creating a package of this with npm pack and then extracting the tarball and injecting it in a master branch version of our client, I am getting the following error:

$ ./bin/cli.js --network=mainnet
ERROR [06-29|16:22:46] uncaughtException: Cannot find module '../../package.json'
Error: Cannot find module '../../package.json'
    at Function.Module._resolveFilename (module.js:548:15)
    at Function.Module._load (module.js:475:25)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/node_modules/ethereumjs-devp2p/dist/src/rlpx/rlpx.js:98:22)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)

This actually seems related to this injected src folder thing, since package.json is looked for one folder too deep (this was happening on a completely fresh install of your fork/branch + all the dependencies reinstalled + fresh build). Maybe really try to address this and remove the src folder, should likely solve this issue as well.

@holgerd77
Copy link
Member

left a comment

Ok, have done side-by-side code reviews of the dpt, eth and les folders, looks good, some minor questions or clarifications.

has(obj: any): boolean {
return KBucket.getKeys(obj).some((key: string) => Boolean(this.lru.get(key)))
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

Looks good.

const debug = createDebugLogger('devp2p:dpt')

export class DPT extends EventEmitter {
privateKey: Buffer

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

Why is privateKey not made private here?


for (let peer of peers) this._server.findneighbours(peer, randomBytes(64))
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

One question on this file, rest looks good.

const peer = this.get(obj)
if (peer !== null) this._kbucket.remove(peer.id)
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

Looks good.

const publicKey = secp256k1.recover(sighash, signature, recoverId, false)

return { typename, data, publicKey }
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

Really nice to have all these additional in/out types here, looks good.

}
}, this._timeout)
}
if (this._socket && peer.udpPort)

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

For the record: is this solving some significant bug or unnecessary communication failures?

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

(the if clause is new)

This comment has been minimized.

Copy link
@dryajov

dryajov Jun 30, 2019

Author

Hmm, I don't think I've added this, but I'll double check.

}
}
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

One question, looks good.

GET_RECEIPTS = 0x0f,
RECEIPTS = 0x10,
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

MSG_CODES moved to its own ETH namespace at the end of the file, looks good.

GET_TX_STATUS = 0x14,
TX_STATUS = 0x15,
}
}

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Jun 29, 2019

Member

Same here, MESSAGE_CODES moved to its own LES namespace at the end of the file, looks good.

@dryajov

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

When I am creating a package of this with npm pack and then extracting the tarball and injecting it in a master branch version of our client, I am getting the following error:

Hmm, yes I believe this is related to the fact that the package.json needs to be copied along with the sources because the version of the lib is extracted from it with import/require, this has already caused some headache, this is also what's forcing the dist/src structure. Can you make sure that the packages structure is correctly replicated when you manually copy the contents over, it has to have the dist folder along with the package.json file inside. I'm afk right now but will try to reproduce asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.