feat: bump aegir and add types #111
Conversation
There was a problem hiding this comment.
Thanks for the PR @acolytec3 ! It was good to start with a simplest module 👍🏼
Left some feedback so that we can have this merged
.gitignore
Outdated
| @@ -34,7 +34,6 @@ build | |||
| node_modules | |||
|
|
|||
| lib | |||
| dist | |||
There was a problem hiding this comment.
The dist should be ignored. The dist folder will be created on the release script for publishing but should not be part of the repo.
There was a problem hiding this comment.
So in this case, do we leave the package.json pointing to dist/src/index.d.ts even though it's no longer included in my PR?
There was a problem hiding this comment.
Yes, it will be created in the release and what is shipped to npm will include the dist folder
src/index.js
Outdated
| @@ -6,8 +6,7 @@ const mafmt = require('mafmt') | |||
| const { EventEmitter } = require('events') | |||
| const debug = require('debug') | |||
|
|
|||
| const log = debug('libp2p:bootstrap') | |||
| log.error = debug('libp2p:bootstrap:error') | |||
| const error = debug('libp2p:bootstrap:error') | |||
There was a problem hiding this comment.
For keeping consistent with other libp2p repos, I suggest changing this to:
const debug = require('debug')
const log = Object.assign(debug('libp2p:bootstrap'), {
error: debug('libp2p:bootstrap:error')
})What do you think? We currently do not use the the normal log, but perhaps we should log when the bootstrap starts.
There was a problem hiding this comment.
This makes sense. I added a log message to the start method in my latest commit.
|
@vasco-santos I think I've addressed all the questions. One thing I wasn't sure about is whether I should still include the emitted types in my PR or leave that out since it sounds like that's part of a separate release process. Do those get added when you do the version bump on npm? |
Yes, when we run the release script, this will use https://github.com/ipfs/aegir#releasing which on build will create the types in the dist folder. As a result, they will be included in npm and will be fetched when libp2p-bootstrap is installed, but will not exist in this repo |
@vasco-santos js-libp2p-crypto was proving to be too big a push for me to do at once so thought I'd take a stab at a more manageable types upgrade so have attempted to follow the patterns linked in previous PRs around adding types via aegir/tsdoc. I've made some minor updates to address typescript specific type safety concerns.