Skip to content

feat: update to new libp2p interfaces #134

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

Merged

Conversation

achingbrain
Copy link
Collaborator

Updates this module to use the @libp2p/* set of modules as part of the typescript port.

  • Renames imports to add .js suffix (tsc does not add this during compilation to ESM)
  • Renames bundled @types/*.d.ts to @types/*.ts because tsc ignores .d.ts which masks errors
  • Updates project config to output ESM instead of CJS code
  • Updates all deps to lastest versions including ones that have gone ESM-only
  • Re-generates protobuf code with the ES6 target
  • Fixes all linting errors
  • Swaps bl for Uint8ArrayList

N.b. pbjs generates invalid imports for bundler-less ESM - the .js suffix has to be added to the import manually so it's best not rebuild the protobuf generated code unless you know something has changed.

BREAKING CHANGE: This module now outputs ESM code only

Updates this module to use the `@libp2p/*` set of modules as part
of the typescript port.

- Renames imports to add .js suffix
- Renames bundled `@types/*.d.ts` to `@types/*.ts` because tsc ignores `.d.ts` which masks errors
- Updates project config to output ESM instead of CJS code
- Updates all deps to lastest versions including ones that have gone ESM-only
- Re-generates protobuf code with the ES6 target

N.b. `pbjs` generates invalid imports for bundler-less ESM - the `.js` suffix
has to be added to the import manually so it's best not rebuild the protobuf
generated code unless you know something has changed.

BREAKING CHANGE: This module now outputs ESM code only
@achingbrain
Copy link
Collaborator Author

achingbrain commented Mar 15, 2022

Running the benchmarks the performance is similar, slightly slower:

New:

$ node benchmark.js 
Initializing handshake benchmark
Init complete, running benchmark
handshake x 52.53 ops/sec ±16.17% (72 runs sampled)

Old:

$ node benchmark.js 
Initializing handshake benchmark
Init complete, running benchmark
handshake x 58.42 ops/sec ±15.39% (77 runs sampled)

@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2022

Codecov Report

Merging #134 (93b1c16) into master (790b039) will decrease coverage by 4.70%.
The diff coverage is 76.81%.

❗ Current head 93b1c16 differs from pull request most recent head e78bfd9. Consider uploading reports for the commit e78bfd9 to get more accurate results

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
- Coverage   85.91%   81.21%   -4.71%     
==========================================
  Files          16       26      +10     
  Lines        1839     2864    +1025     
  Branches      235      279      +44     
==========================================
+ Hits         1580     2326     +746     
- Misses        259      538     +279     
Impacted Files Coverage Δ
test/noise.spec.ts 52.34% <48.91%> (ø)
test/handshakes/xx.spec.ts 90.62% <55.55%> (ø)
src/handshakes/abstract-handshake.ts 89.65% <57.14%> (-2.43%) ⬇️
test/ik-handshake.spec.ts 55.95% <68.42%> (ø)
test/handshakes/ik.spec.ts 95.83% <71.42%> (ø)
src/proto/payload.js 78.64% <78.43%> (-1.08%) ⬇️
src/utils.ts 90.66% <80.00%> (-7.74%) ⬇️
test/utils.ts 90.00% <80.00%> (ø)
src/noise.ts 76.22% <83.33%> (-0.29%) ⬇️
test/xx-fallback-handshake.spec.ts 93.58% <85.71%> (ø)
... and 38 more

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 790b039...e78bfd9. Read the comment docs.

@dapplion
Copy link
Contributor

Thank you so much! before merging this Lodestar must switch to ESM too, so we'll wait for that to happen

@achingbrain
Copy link
Collaborator Author

I'm not sure it's necessary to wait, this can (and should) be published as a major, Lodestar can use the previous version until it's ready to upgrade?

Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks good to me! @achingbrain yeah you are right we can merge this and keep using the current version is Lodestar. And backport fixes if necessary.

(source: Iterable<Uint8Array>): AsyncIterableIterator<Uint8Array>
}
import type { Transform } from 'it-stream-types'
import type { IHandshake } from './@types/handshake-interface.js'
Copy link
Member

Choose a reason for hiding this comment

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

This is so weird. Is there a reason why .d.ts is not ok anymore like it should not be compiled to js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From the way I understand it, .d.ts files are meant to be machine generated so they aren't parsed and validated like regular .ts files.

We've certainly shipped a lot of broken types in the past due to hand-rolled .d.ts files - things like import paths being wrong etc don't trigger errors so it's a lot safer this way.

@mpetrunic mpetrunic merged commit 732e468 into ChainSafe:master Mar 16, 2022
@achingbrain achingbrain deleted the feat/update-to-new-interfaces-and-esm branch March 16, 2022 15:10
@achingbrain
Copy link
Collaborator Author

Will this get a release soon? It's the last piece of the puzzle before I can open the ts PR for libp2p itself.

@mpetrunic
Copy link
Member

will do now

@mpetrunic
Copy link
Member

Will this get a release soon? It's the last piece of the puzzle before I can open the ts PR for libp2p itself.

https://github.com/ChainSafe/js-libp2p-noise/releases/tag/v6.0.0

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