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

Revert host overriding functionality #353

Closed
wants to merge 2 commits into from
Closed

Conversation

g7s
Copy link

@g7s g7s commented Nov 4, 2019

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

@Deraen
Copy link

Deraen commented Nov 6, 2019

@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
Copy link
Author

g7s commented Nov 6, 2019

@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
Copy link

Deraen commented Nov 7, 2019

There is no hostname option.

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

@g7s
Copy link
Author

g7s commented Nov 7, 2019

@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
Copy link
Contributor

currentoor commented Nov 18, 2019

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.

@currentoor currentoor mentioned this pull request Dec 8, 2019
@ptaoussanis
Copy link
Member

Addressing now via #366 - apologies for the delay on this.

@currentoor
Copy link
Contributor

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

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.

4 participants