-
Notifications
You must be signed in to change notification settings - Fork 460
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
avoid constructor.name . fix for #65 #177
Conversation
@@ -129,7 +130,7 @@ class Node extends EventEmitter { | |||
this.switch.transport.add( | |||
transport.tag || transport.constructor.name, transport) | |||
} else if (transport.constructor && | |||
transport.constructor.name === 'WebSockets') { | |||
transport instanceof WebSockets) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback as multiformats/js-multiaddr#55 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diasdavid hey,
solution 1 : What if we add a name
property to the WebSockets
class which isn't gonna be mangled by uglify like so
class WebSockets {
constructor (opts) {
this.name = 'WebSockets'
}
}
Solution 2 : use both instanceOf
and constructor,name
like so
if (transport.constructor.name === 'WebSockets' || transport instanceOf WebSockets)
What do you think.??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solution 1 is closer to a real solution. Even better would be just to have a isWebSockets
method that checks multiple things
See @pgte proposal ipfs/js-ipfs#1131 (comment) |
PR #182 fixes this. |
This has been fixed in all the js-ipfs deps thanks to @fsdiogo. @ya7ya thanks for helping us diagnose and provide solutions. Check ipfs/js-ipfs#1321 for more info. |
this avoids using
constructor.name
so uglify can mangle and compressjs-ipfs
properly.this should close #65
cc @diasdavid