Skip to content
This repository has been archived by the owner on Aug 24, 2021. It is now read-only.

feat: add ts types #72

Merged
merged 10 commits into from
Nov 20, 2020
Merged

feat: add ts types #72

merged 10 commits into from
Nov 20, 2020

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented Nov 18, 2020

needs ipfs/aegir#671

closes #54
closes #29

check published docs https://multiformats.github.io/js-multibase

Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Mostly nits & suggestions. Only thing I do feel strongly about is type names that I think should be singular instead of plural.

I also flagged out few things with ⚠️ which appear to be breaking changes and might be worth reconsideration.

With type names changed 👍from me and no need for another review cycle.

.github/workflows/main.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -72,55 +57,6 @@ console.log(decodedBytes.toString())
## API
https://multiformats.github.io/js-multibase/
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Maybe we do not need the whole section just for the link ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggestions ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t know maybe API Docs badge at the top ?

package.json Outdated Show resolved Hide resolved
src/base.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, just needs the gh url for aegir removing.

@hugomrdias hugomrdias merged commit 6e083f0 into master Nov 20, 2020
@hugomrdias hugomrdias deleted the feat/ts-types branch November 20, 2020 12:09
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.

typescript support
3 participants