search

taoensso / sente

Realtime web comms library for Clojure/Script
https://www.taoensso.com/sente
Eclipse Public License 1.0
1.73k stars 193 forks source link

Revert host overriding functionality #353

Closed g7s closed 4 years ago

g7s commented 4 years ago

A previous PR broke the host overriding functionality. This commit fixes it.

Deraen commented 4 years ago

@ptaoussanis This is important. Sente is currently unusable e.g. in React-native apps, as window location doesn't mean anything.

But I think this fix should also support case where both :host and :port are set? Or at least the docstring should be updated to reflect that :port is only useful when connecting to current host, but with different port.

g7s commented 4 years ago

@Deraen You can provide the port as part of the :host and it works. On the other hand it should support :hostname and :port options (not only port) because as you pointed out there are environments without a window location.

Deraen commented 4 years ago

There is no hostname option.

Doesn't matter if host option can include port, port option shouldn't prevent using host.

g7s commented 4 years ago

@Deraen Sure there is no hostname option, I was just proposing it. I pushed another commit that when port option is present it overrides the port part of the host or inserts the port if none present.

currentoor commented 4 years ago

I can confirm this totally breaks Sente for React Native. Also currently React Native users have to set a system property to build their projects, which is undesirable with tools like shadow-cljs which re-use the same JVM process for multiple builds simultaneously.

In this comment, I proposed a solution that doesn't require setting a system property.

ptaoussanis commented 4 years ago

Addressing now via https://github.com/ptaoussanis/sente/pull/366 - apologies for the delay on this.

currentoor commented 4 years ago

No worries at all @ptaoussanis, thanks for the awesome library!