Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Port to typescript 2 #56

Merged
merged 30 commits into from
Apr 2, 2020
Merged

Port to typescript 2 #56

merged 30 commits into from
Apr 2, 2020

Conversation

dryajov
Copy link
Contributor

@dryajov dryajov commented Mar 30, 2020

Continuation of #51

@dryajov dryajov mentioned this pull request Mar 30, 2020
@lgtm-com
Copy link

lgtm-com bot commented Mar 30, 2020

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

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

@dryajov
Copy link
Contributor Author

dryajov commented Mar 30, 2020

@holgerd77 @PhilippLgh this seems to be passing now, is there anything else missing?

@PhilippLgh PhilippLgh self-requested a review March 31, 2020 14:33
@holgerd77
Copy link
Member

@PhilippLgh We don't need to directly integrate a new Common version here but can you short-term open a PR with an update on the bootnodes? Then it is at least possible to manually take over the bootnodes from there and run and test the examples here from the library.

We can then do a separate PR here integrating a new Common version (and likely also a few others 😄).

@holgerd77
Copy link
Member

(or - third version - to speed things up if you are eager to merge: you can also do a local example run and just paste the result here, this just need to get through to block/tx sync in some decent amount of time)

@PhilippLgh
Copy link
Contributor

Just finished the bootnode PR: ethereumjs/ethereumjs-common#83

@PhilippLgh
Copy link
Contributor

PhilippLgh commented Mar 31, 2020

tested the examples locally with a linked updated version of ethereumjs-common and it seems to be fine... though I'm not 100% sure what output to expect

npm link "ethereumjs-common"
npm run build
node dist/examples/simple.js
node dist/examples/peer-communication.js

moved examples to src. everything else seems to be extremely complicated if not impossible: microsoft/TypeScript#9858 (comment)

two minor improvements would be to have package.json scripts for examples and remove examples from production build...

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2020

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

new alerts:

  • 1 for Useless conditional

fixed alerts:

  • 1 for Useless conditional

@holgerd77
Copy link
Member

@PhilippLgh Thanks! There are still some linting errors in the example files, can you fix?

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed a last time through the code changes, from my side this looks good now. @PhilippLgh feel free to merge once you are satisfied with the changes and feel this is ready.

Dmitriy @dryajov Thanks again for all the hard work here, so great! 😀 Beside you also really deserve some Outstanding-Patience-Award here for ever again keeping on with this after such a long period of time.

Thanks! 🙏

@holgerd77
Copy link
Member

For a release I would suggest we give this another let's say 2 weeks of time, eventually we can target April 15, Wednesday, or eventually the Tuesday. Philipp wanted to give this some more maintenance work and during this time the dust can settle a bit, eventually Philipp also stumbles upon some more glitches while just using the library and doing updates and active development work.

@holgerd77
Copy link
Member

(no totally fix date though, just as some orientation so this won't get lost again)

@PhilippLgh
Copy link
Contributor

would merge this and follow the proposed two weeks game plan

Copy link
Contributor

@PhilippLgh PhilippLgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@PhilippLgh PhilippLgh merged commit 5d10059 into master Apr 2, 2020
@PhilippLgh PhilippLgh deleted the feat/typescript branch April 2, 2020 13:19
@holgerd77 holgerd77 mentioned this pull request Apr 24, 2020
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants