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

Fix for React Native #357

Closed currentoor closed 4 years ago

currentoor commented 4 years ago

This PR includes the fix from https://github.com/ptaoussanis/sente/pull/353, since it's necessary for React Native development.

I've verified this work in React Native, android and ios, for dev and production builds. Also addresses https://github.com/ptaoussanis/sente/issues/247.

I think this approach is better since it does not require setting an environment variable. So the elide-require call can probably go away.

Also, thanks for this awesome library!

awkay commented 4 years ago

So, it seems that the system val (env or property) SENTE_ELIDE_JS_REQUIRE is what activates the elide require logic @currentoor ; however, I would vote for this patch anyway @ptaoussanis because we often use sente in projects that span node + native + web and use shadow-cljs to build them all...one JVM process (often run in server mode for speed of switching gears), and system value does not work well with this. Env variables are a complete no-go, and while I personally usually prefer system properties: in this case I would really rather use this patch because it simply works no matter what....I would delete the elide require stuff just to prevent confusion.

I'm about to deploy an update to Fulcro that uses sente to talk to external dev tools in React Native...people can use the existing system property, but when they also have a web build they will run into the same nightmare.

mitchelkuijpers commented 4 years ago

We are currently unable to use fulcro-inspect because we cannot override the host unfortunately :( Is there a reason why this fix isn't being merged?

currentoor commented 4 years ago

@mitchelkuijpers I have a temporary fork here with this fix. We are in the same boat.

org.clojars.currentoor/sente       {:mvn/version "1.14.1-SNAPSHOT"}
wilkerlucio commented 4 years ago

Just noticed that this seems a long-running issue, ended up just opening a new PR for it, very simple, just to allow the host back #370

evelant commented 4 years ago

This is still an issue. To use fulcro-inspect electron with react-native android on windows I overrode the sente dep in fulcro in deps.edn like so

org.clojars.currentoor/sente      {:mvn/version "1.14.1-SNAPSHOT"}
com.fulcrologic/fulcro              {:mvn/version "3.1.8" :exclusions [com.taoensso/sente]}

Then added the following to :closure-defines in shadow-cljs.edn (Android studio emulator points 10.0.2.2 to the host machine, this isn't necessary for iOS emulator)

com.fulcrologic.fulcro.inspect.inspect_ws/SERVER_PORT 8237
 com.fulcrologic.fulcro.inspect.inspect_ws/SERVER_HOST "10.0.2.2"

The electron version of fulcro-inspect works fine after that change.

ptaoussanis commented 4 years ago

Merging manually now, thank you Tom! Apologies for the delay.