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

Refactor to use Typescript #77

Merged
merged 9 commits into from
Jun 3, 2020
Merged

Refactor to use Typescript #77

merged 9 commits into from
Jun 3, 2020

Conversation

wemeetagain
Copy link
Member

@wemeetagain wemeetagain commented May 29, 2020

Refactored to use typescript with minimal changes to process and code.
Imo, using typescript in this project will pay dividends as the gossipsub feature-set grows. It becomes much easier to reason about this codebase which has many moving parts and datatypes when there's that minimal level of type/sanity checking.
That said, I attempted to do this update in a way that's minimally invasive to the standard libp2p workflow.

  • typescript code lives in ts/, gets transpiled to src/
    • prebuild and pretest scripts runs tsc
    • build step left untouched - aegir build
    • emits js code in strict mode
    • pubsub.js left untouched (expected to migrate to js-libp2p-pubsub repo eventually)
    • src/ not checked into git, like dist/
  • built dist/index.min.js is 2KB larger, from 468KB to 470KB
  • linting now performed by eslint (using standard and @typescript-eslint configs/plugins) instead of aegir, as the typescript files themselves are to be linted
  • tests left untouched
  • caught a few bugs
    • doc strings out of sync
    • no null checks for nullish values
    • improper use of functions due to misleading variable names
  • GossipSub => Gossipsub throughout (I can remove to future PR if desired)
  • types in ts/peer.ts should eventually go to js-libp2p-pubsub

@wemeetagain wemeetagain requested review from jacobheun, vasco-santos and a team May 29, 2020 16:31
@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #77 into master will decrease coverage by 5.68%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   85.68%   80.00%   -5.69%     
==========================================
  Files          11        1      -10     
  Lines         510       10     -500     
==========================================
- Hits          437        8     -429     
+ Misses         73        2      -71     
Impacted Files Coverage Δ
test/utils/index.js 80.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d31804...666099e. Read the comment docs.

Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

Since you are using typescript it would be useful if you add public/private/protected modifiers so we can navigate easily when extending in lodestar

.eslintrc.js Outdated Show resolved Hide resolved
ts/getGossipPeers.ts Show resolved Hide resolved
ts/message/index.ts Show resolved Hide resolved
ts/message/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/peer.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
@wemeetagain
Copy link
Member Author

@vasco-santos Thanks for bringing up the larger issue of standardizing the tooling and strategy for supporting typescript across the js-libp2p/js-ipfs ecosystem.

I do think that using full typescript here will be beneficial, over, eg: autogen declarations. Imo it mainly boils down to improved dev tooling and type-checking sanity checks. I think the benefits outweigh the additional machinery and differences with other libp2p repos.

The current gossipsub codebase has many rough edges and using typescript here clears up many ambiguities. As part of this process, several bugs were found just from having type-checking. While these bugs were obvious in hindsight, those particular issues would have never passed the base level of scrutiny we get using typescript. I'd love to see this gossipsub implementation be polished and be easily shown to be correct, and I think type-checking plays a role here.

The dev tooling aspect is also useful to call out. I've been working on gossipsub v1.1 in a local branch, and having the full IDE type-checking/hinting and autocompletion significantly reduces the mental burden of eg: spelling out logic with app-specific datastructures. We're able to declare datastructures with interfaces (in the case of gossipsubv1.1, these interfaces can be fairly large and wordy) and seamlessly use these to aid development without having to navigate opaque objects by hand. Using vanilla js, I was struggling to have my IDE link JSDocs throughout the project, and having to manually check for typos and correct usage, so the experience was not nearly so pleasant.

Remaining thoughts are how to best do this PR w/o negatively impacting the js-libp2p ecosystem and with maximal standardization/collaboration with the js-libp2p ecosystem.

I would love to keep using aegir where possible, (using it for linting eventually, if/when supported), and the current structure is an attempt to simply extend the current processes to a typescript codebase. Ideally after ts => js transpilation/pre-processing, "things can be treated the same".

Perhaps this may be the only js-libp2p repo using typescript, but I would like to get this right to maximally align with the whole ecosystem so it doesn't have to be treated strangely and can benefit from standardization.

@vasco-santos
Copy link
Collaborator

@wemeetagain thanks for exposing your thoughts. I agree with your thoughts and we should move on the way you are working on!

Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM!

@wemeetagain wemeetagain merged commit 49dc8fd into master Jun 3, 2020
@wemeetagain wemeetagain deleted the typescript branch June 3, 2020 15:11
@wemeetagain wemeetagain mentioned this pull request Jun 3, 2020
@vasco-santos
Copy link
Collaborator

FYI, just released 0.4.3 with this PR and a custom aegir on my local machine to skip the dirty validation

fryorcraken pushed a commit to fryorcraken/js-libp2p-gossipsub that referenced this pull request Aug 2, 2022
Pass `maxInboundStreams` and `maxOutboundStreams` options to registrar
fryorcraken pushed a commit to fryorcraken/js-libp2p-gossipsub that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants