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

feat: use class-is module for type checks #61

Merged
merged 1 commit into from
Mar 30, 2018

Conversation

fsdiogo
Copy link
Member

@fsdiogo fsdiogo commented Mar 28, 2018

This is an ongoing effort to fix the ipfs/js-ipfs#938 issue.

It uses a module I've published that abstracts this solution: ipfs/js-ipfs#1131 (comment).

@fsdiogo fsdiogo requested a review from daviddias March 28, 2018 15:14
@fsdiogo fsdiogo self-assigned this Mar 28, 2018
@vmx
Copy link
Member

vmx commented Mar 29, 2018

@fsdiogo Do I understand it correctly that isMultiaddr() is automatically provided by to the class-is module (I was wondering why the whole function was deleted)?

@fsdiogo
Copy link
Member Author

fsdiogo commented Mar 30, 2018

@vmx yes, you got it! That’s why I removed that function.

@vmx vmx merged commit b097af9 into master Mar 30, 2018
@vmx vmx deleted the feat/use-class-is-module-for-type-checks branch March 30, 2018 21:41
@daviddias
Copy link
Member

Let's wait on merging things to wait @fsdiogo confirming that the solution is complete and sound.

@@ -44,7 +43,7 @@ function Multiaddr (addr) {
} else {
throw new Error('addr must be a string, Buffer, or another Multiaddr')
}
}
}, { className: 'Multiaddr', symbolName: '@multiformats/js-multiaddr/multiaddr' })
Copy link
Member

Choose a reason for hiding this comment

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

@fsdiogo instead of adding it at the top (which makes it look like a 👽 function declaration for folks non familiar with the whole situation), do the withIs.proto a separate statement before the module exports.

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.

3 participants