Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Add type generation from jsdoc #64

Merged
merged 15 commits into from
Sep 30, 2020

Conversation

mpetrunic
Copy link
Member

@mpetrunic mpetrunic commented Sep 4, 2020

Related to: libp2p/js-libp2p#659

I've fixed some jsdocs to generate better types. There is also some issue with index modules but overall jsdoc seems to be
generating quite good types. A lot of stuff related to protons library is "any" since that library doesn't have types. Not sure from where to import/use all those stream/duplex/DuplexIterable types

Do we wan't some precommit hook to regenerate types?

cc @wemeetagain @tuyennhv you can try from this branch directly

index.d.ts Outdated Show resolved Hide resolved
@mpetrunic
Copy link
Member Author

cc @vasco-santos is this proper way to include types?

@vasco-santos
Copy link
Member

@mpetrunic The goal is to not have the index.d.ts in the repo. aegir should generate it from the JS docs when we release each module. This way, it is always updated with the current jsdoc. However, it seems that aegir did not release yet the support for it ipfs/aegir#568

@hugomrdias do you have any visibility on when you will be able to go back to the aegir support?

@vasco-santos
Copy link
Member

If we do not have support soon for aegir, we can move forward with this as the migration should be as simple as removing the declaration file from the repo and aegir generate it. @mpetrunic let us know once we should review this PR

@hugomrdias
Copy link
Member

@mpetrunic The goal is to not have the index.d.ts in the repo. aegir should generate it from the JS docs when we release each module. This way, it is always updated with the current jsdoc. However, it seems that aegir did not release yet the support for it ipfs/aegir#568

@hugomrdias do you have any visibility on when you will be able to go back to the aegir support?

no i don't, it will take a while probably

@mpetrunic
Copy link
Member Author

mpetrunic commented Sep 16, 2020

@vasco-santos
Seems like we can export single type declaration only if we export everything from src/index.js file and use that only. So I changed tsconfig to generate type declaration alongside js sources.

@wemeetagain @tuyennhv
Imports work now, but gossipsub has a lot of duplicated/incompatible types with libp2p-interfaces. We should probably try to port PeerStreams type definition into jsdoc here.

@wemeetagain
Copy link
Member

gossipsub has a lot of duplicated/incompatible types with libp2p-interfaces

👍 the types there are meant to be temporary until these types (and js-libp2p core) types are available. Happy to slowly get rid of these libp2p types duplicated everywhere 😂
@tuyennhv has a branch https://github.com/ChainSafe/js-libp2p-gossipsub/compare/tuyen/libp2p-interfaces-types-2 where we can sort this out.

@vasco-santos
Copy link
Member

vasco-santos commented Sep 17, 2020

Seems like we can export single type declaration only if we export everything from src/index.js file and use that only. So I changed tsconfig to generate type declaration alongside js sources.

Yeah, that should be because we require specific interfaces

@mpetrunic should I review the PR or should we wait on gossipsub integration before?

@hugomrdias
Copy link
Member

@Gozala has taken over the ts jsdocs aegir PRs and is making good progress. Maybe that helps here.

@mpetrunic
Copy link
Member Author

Seems like we can export single type declaration only if we export everything from src/index.js file and use that only. So I changed tsconfig to generate type declaration alongside js sources.

Yeah, that should be because we require specific interfaces

@mpetrunic should I review the PR or should we wait on gossipsub integration before?

@vasco-santos I'm still getting types inline with gossipsub and removing duplicates, so pls wait a bit more before reviewing.

@Gozala has taken over the ts jsdocs aegir PRs and is making good progress. Maybe that helps here.

@hugomrdias We can switch over once it's ready, I'm mostly changing jsdoc to describe types more accurately.

src/pubsub/index.js Outdated Show resolved Hide resolved
src/pubsub/index.js Outdated Show resolved Hide resolved
@twoeths
Copy link

twoeths commented Sep 23, 2020

Confirm that gossipsub works fine with this PR 👍

@mpetrunic
Copy link
Member Author

@vasco-santos we are ready for review :)

Copy link
Member

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

Overall this looks good to me. Just minor stuff below

package.json Outdated Show resolved Hide resolved
src/connection/connection.js Show resolved Hide resolved
src/connection/connection.js Show resolved Hide resolved
@wemeetagain
Copy link
Member

I think this is ready for re-review

Copy link
Member

@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! 🎉

@vasco-santos vasco-santos merged commit eacdc24 into libp2p:master Sep 30, 2020
@vasco-santos
Copy link
Member

FYI Shipped libp2p-interfaces@0.5.2

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.

5 participants