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

fix!: update to esm only export #236

Merged
merged 35 commits into from
May 10, 2022

Conversation

achingbrain
Copy link
Collaborator

@achingbrain achingbrain commented Apr 26, 2022

  • Updates all deps to @libp2p/* versions
  • Reference types (Message, signing, etc) in @libp2p/interfaces instead of redefining them
  • Updates config to output ESM
  • Makes the GossipSub class implement the PubSub interface so it can be used with libp2p@next (e.g. instead of extending PubSubBaseProtocol)
  • Removes the libp2p constructor arg as the components are injected by libp2p at runtime
  • Swaps protobufjs for protons

BREAKING CHANGE: the output of this module is now ESM-only

- Updates all deps to `@libp2p/*` versions
- Updates config to output ESM
- Makes the `GossipSub` class implement the `PubSub` interface so it can be used with `libp2p@next`
- Removes the `libp2p` constructor arg as the components are injected by `libp2p` at runtime
- Swaps protobufjs for protons

BREAKING CHANGE: the output of this module is now ESM-only
@achingbrain achingbrain requested a review from a team as a code owner April 26, 2022 11:21
@achingbrain
Copy link
Collaborator Author

achingbrain commented Apr 26, 2022

Some thoughts:

  • I had to make a load of private methods public in the GossipSub class for the tests to compile. I figure this isn't the end of the world since at runtime libp2p.pubsub uses the PubSub interface for the public API and not GossipSub
  • Because libp2p is no longer responsible for the lifecycle of it's components (e.g. the user news them up instead of passing constructors & constructor args in which are essentially impossible to type), we can't pass a libp2p node in to the GossipSub constructor any more, instead it now gets the components it needs by being Initializable. The tests used to access the .libp2p prop of GossipSub but now they use instances of libp2p and access GossipSub via libp2p.pubusb instead so it's been flipped around. The access of .libp2p in the tests is mostly used to connect GossipSub instances together - longer term this should be refactored to do the same thing via the Registrar component (see the pubsub interface compliance tests for an example) - this is much lighter weight, is faster and means this module doesn't need to depend on libp2p, transports, encryption, multiplexers, etc, etc, any more - Update - I have done this in 6d36b4b, the tests are now a lot faster.

Before:

  gossip
    ✅ should send piggyback control into other sent messages (6860ms)
  1 passing (8s)

After:

  gossip
    ✅ should send piggyback control into other sent messages (646ms)
  1 passing (1s)
  • This module uses aegir for (I think) pretty much just releases and building a browser bundle now, since it has it's own eslint rules, ts configuration etc, so it's getting a lot of the costs of aegir with very little of the benefit. I think it should either be all-in on aegir or shouldn't use it at all. I am more than happy to add extra linting/ts rules to aegir as required (enforcing return types on methods is definitely something we should add, for example). The rest of the rules don't seem all that much different so I'm not sure why adopting a different set of eslint rules was necessary.
  • One for a follow-up PR, but this module does a lot of PeerId -> string -> PeerId conversion, mostly to use PeerId as keys in Maps/Sets, etc - it could use the PeerMap and PeerSet classes in @libp2p/peer-collections to do the same thing without all the explicit conversions

@achingbrain achingbrain marked this pull request as draft April 26, 2022 11:40
@achingbrain achingbrain marked this pull request as ready for review April 27, 2022 13:13
@achingbrain achingbrain force-pushed the fix/update-libp2p-deps branch 2 times, most recently from 504de2e to 38c2811 Compare May 2, 2022 10:30
Also, if we do not send a message to a peer, remove them from the recipients list
@wemeetagain
Copy link
Member

Once ESM-only lands, we can rename directories to be consistent with other libp2p repos.

The reason we had source (typescript) files under ts/, and build artifacts (js) files under src/ was to allow consumers to import subdirectories from the "same place" as other libp2p packages. eg: import foo from "libp2p-gossip/src/foo" being similar to import bar from "libp2p/src/bar".
After ESM-only, exports must be named explicitly in package.json, and the src/ subdirectory name no longer bleeds into the export name.

@mpetrunic mpetrunic changed the title fix: update to esm libp2p and deps fix!: update to esm only export May 10, 2022
@mpetrunic mpetrunic merged commit f9a8298 into ChainSafe:master May 10, 2022
@achingbrain achingbrain deleted the fix/update-libp2p-deps branch May 11, 2022 06:11
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.

5 participants