search

ethereumjs / ethereumjs-devp2p

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/devp2p
141 stars 62 forks source link

Port to typescript #51

Closed dryajov closed 4 years ago

dryajov commented 5 years ago

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[bot] commented 5 years ago

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

new alerts:

fixed alerts:

Comment posted by LGTM.com

lgtm-com[bot] commented 5 years ago

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

new alerts:

fixed alerts:

Comment posted by LGTM.com

holgerd77 commented 5 years ago

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

holgerd77 commented 5 years ago

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

dryajov commented 5 years ago

Thanks @holgerd77 - you can see how I'm consuming it in https://github.com/musteka-la/kitsunet-js/pull/29/files#diff-0f43fda173faf5fcb1c8e5d3447d17ac and https://github.com/musteka-la/kitsunet-js/pull/29/files#diff-f5ab575e22486039935e63d6f9ea7b79 .

holgerd77 commented 5 years ago

@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 commented 5 years ago

@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[bot] commented 5 years ago

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

new alerts:

fixed alerts:

Comment posted by LGTM.com

holgerd77 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

krzkaczor commented 5 years ago

My few cents regarding tsconfig changes:

alcuadrado commented 5 years ago
  • 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 commented 5 years ago

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

alcuadrado commented 5 years ago

@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.

lgtm-com[bot] commented 5 years ago

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

new alerts:

fixed alerts:

Comment posted by LGTM.com

dryajov commented 5 years ago

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[bot] commented 5 years ago

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

new alerts:

fixed alerts:

Comment posted by LGTM.com

krzkaczor commented 5 years ago

@dryajov thanks, makes sense!

dryajov commented 5 years ago

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 commented 5 years ago

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

dryajov commented 5 years ago

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

holgerd77 commented 5 years ago

@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[bot] commented 5 years ago

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

new alerts:

fixed alerts:

lgtm-com[bot] commented 5 years ago

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

new alerts:

fixed alerts:

dryajov commented 5 years ago

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[bot] commented 5 years ago

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

new alerts:

fixed alerts:

dryajov commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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.

dryajov commented 5 years ago

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.

holgerd77 commented 5 years ago

@dryajov Back from holidays. 😄 Did you have a chance to have a deeper look at this packaging/distribution error? This is the major thing holding back a release respectively review finalization, would be great if we get all your changes to work soon, looking forward to it!

dryajov commented 5 years ago

I'll pick this back up in the next couple of days.

holgerd77 commented 5 years ago

@dryajov ping 🙂

dryajov commented 5 years ago

Hi @holgerd77 ! Sorry for dropping the ball here - still interested in seeing this completed. AFAIR, the only problem is figuring out the issues with the src folder. It ended up not being trivial to fix and other unrelated things distracted me from digging deeper. Has anyone else been able to take a look at that? I don't think that changing the output folder structure would imply any major reworking, but it certainly needs some tweaking. I'll do my best to look at this next week, but I can't promise much as I'm heads down on something else - that said I'm committed to getting this fixed.

PhilippLgh commented 4 years ago

Hello @dryajov, there have been some very unfortunate delays in the PR review process here due to internal re-orgs et cetera. The PR is awesome and we really want to see it being merged. Just assigned myself to the PR and want to assist in this process. Did a quick test run and it seems there is not much missing to get it merged. Made two comments regarding changes. For the build config I would suggest to either move examples into src or create a separate build target and exclude them from main build. Otherwise the compiler will complain that src files are outside the src folder.

Please let me know how to best proceed from here :)

holgerd77 commented 4 years ago

Hi Dmitriy @dryajov, was great meeting you at EthCC 😄, can only reiterate on Philipps post above. It would be really valuable if you can give a short term reaction here, there are now new requests on the library coming in and we want to pick up development again a bit and all this should likely be done with having this PR merged as a basis.

It would be great if you find some in-between time to integrate these last changes. But it you can't, it would be important that you give this a minute of reflection and then just communicate on your current state. Then we can actively think about an alternative path, eventually Philipp can do the latest additions himself on this branch or we can do an unfinished in-between-merge and do some last-finishing PRs separtely.

All the best, hope everything is fine! Holger

lgtm-com[bot] commented 4 years ago

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

new alerts:

fixed alerts:

holgerd77 commented 4 years ago

Uh. Real time. 😄 ❤️

dryajov commented 4 years ago

@holgerd77, great meeting you at EthCC as well :) Super excited to see this being picked up again and I'm definitely here to assist in any way possible. I would say that @PhilippLgh can probably pick this up from where I left it off and I'm definitely around to give a hand when needed.

For the build config I would suggest to either move examples into src or create a separate build target and exclude them from main build. Otherwise the compiler will complain that src files are outside the src folder.

@PhilippLgh that sounds reasonable, let's move it into src. I've also went ahead an accepted your suggestions.

Also, let me know if we should create a separate PR on this repo as opposed to the fork to facilitate collaborating on it.

holgerd77 commented 4 years ago

@dryajov Great! 😄

I've send you a collaboration invite with write access, if you want to resubmit the PR directly on the repo feel free or otherwise arrange directly with @PhilippLgh on how you guys want to proceed (resubmitting the PR won't hurt though anyhow if it is not too much of a hazzle for you).

PhilippLgh commented 4 years ago

thx @dryajov for the fast response.. we actually might be able to get it done today. the last changes I see as blocker are in the tsconfig. with these changes: src should be built into dist directly. test can be ignored as it does not need compilation - tests are run with tsnode. and examples can be either excluded from build or moved into src.. with that all tests should run and succeed. the examples should mostly work when compiled, however there is still an issue with one of the libraries that has to be tackled separately

holgerd77 commented 4 years ago

@dryajov Cool, thanks! There are some few linting errors in:

src/dpt/server.ts src/rlpx/rlpx.ts tsconfig.prod.json

Can you fix these? Then we don't have to admin-merge this.

dryajov commented 4 years ago

closing in favor of https://github.com/ethereumjs/ethereumjs-devp2p/pull/56

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert and fixes 1 when merging 2273badbc9a782787ef45e7c851c6d8ed5fde2a6 into 22c9a1c3f1be9c218b0917a90f5ae9fed2696978 - view on LGTM.com

new alerts:

fixed alerts: