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

Checking for WebSockets constructor name fails under uglification #65

Closed
jackkleeman opened this Issue Mar 26, 2017 · 8 comments

Comments

6 participants
@jackkleeman

jackkleeman commented Mar 26, 2017

Currently to add the WebSockets transport index.js does a check for the constructor.name being 'WebSockets'. This fails under uglification unless 'WebSockets' is made a reserved word in the uglify config, as constructor names are one of the things uglify removes. I recommend that either this is documented, or a different solution is devised; there is already a TODO in the code suggesting that this is found. For example, we could standardise some kind of boolean flag in a transport class which tells libp2p to include it even if there are no listening addresses. That wouldn't be websocket-specific, and wouldn't require checking the name against a string.

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Mar 26, 2017

Member

This is unfortunate, we use this pattern in a couple of places. I guess we need to find a new way to do these kinds of detections.

Member

dignifiedquire commented Mar 26, 2017

This is unfortunate, we use this pattern in a couple of places. I guess we need to find a new way to do these kinds of detections.

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Mar 27, 2017

Member

Trying to come up with an elegant solution and I always end up in making something that looks too complicated. The crux of this problem is the fact that we might handle several versions of the same module that are compatible with each other, but because of the dependency tree, they coexist.

Seems that we need to go 🦆 typing all the way, which isn't the best, but will suffice for the majority (if not all) the cases.

Member

diasdavid commented Mar 27, 2017

Trying to come up with an elegant solution and I always end up in making something that looks too complicated. The crux of this problem is the fact that we might handle several versions of the same module that are compatible with each other, but because of the dependency tree, they coexist.

Seems that we need to go 🦆 typing all the way, which isn't the best, but will suffice for the majority (if not all) the cases.

@mqzry

This comment has been minimized.

Show comment
Hide comment
@mqzry

mqzry Aug 9, 2017

Warning: I did not look at the code yet.
Can't you just specify a regex to the mangle-regex option of uglify that matches functions starting with a capital? Not the most robust solution though.

Can you explain the crux of the problem a bit more? I might try to solve this issue

mqzry commented Aug 9, 2017

Warning: I did not look at the code yet.
Can't you just specify a regex to the mangle-regex option of uglify that matches functions starting with a capital? Not the most robust solution though.

Can you explain the crux of the problem a bit more? I might try to solve this issue

@dryajov

This comment has been minimized.

Show comment
Hide comment
@dryajov

dryajov Aug 9, 2017

Member

Actually libp2p (as well as libp2p-swarm) uses the .tag property as the primary way to determine the name of the transport, it uses .constructor.name only as a fallback, so it should be possible to just set the .tag as the name of the transport. I like this solution and I explicitly set .tag to Circtuit in libp2p-circuit, for example.

Member

dryajov commented Aug 9, 2017

Actually libp2p (as well as libp2p-swarm) uses the .tag property as the primary way to determine the name of the transport, it uses .constructor.name only as a fallback, so it should be possible to just set the .tag as the name of the transport. I like this solution and I explicitly set .tag to Circtuit in libp2p-circuit, for example.

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Apr 3, 2018

Member

@fsdiogo is handling this.

Member

diasdavid commented Apr 3, 2018

@fsdiogo is handling this.

@fsdiogo

This comment has been minimized.

Show comment
Hide comment
@fsdiogo

fsdiogo Apr 3, 2018

Member

PR #182 fixes this.

Member

fsdiogo commented Apr 3, 2018

PR #182 fixes this.

@fsdiogo

This comment has been minimized.

Show comment
Hide comment
@fsdiogo

fsdiogo Apr 23, 2018

Member

Check ipfs/js-ipfs#1321 for more info.

Member

fsdiogo commented Apr 23, 2018

Check ipfs/js-ipfs#1321 for more info.

@fsdiogo fsdiogo closed this Apr 23, 2018

@wafflebot wafflebot bot removed the backlog label Apr 23, 2018

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Apr 24, 2018

Member

WoooT!

Member

diasdavid commented Apr 24, 2018

WoooT!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment