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

Unable to resolve module websocket in react native release mode #247

Closed
tiensonqin opened this issue Jul 15, 2016 · 18 comments
Closed

Unable to resolve module websocket in react native release mode #247

tiensonqin opened this issue Jul 15, 2016 · 18 comments

Comments

@tiensonqin
Copy link
Contributor

Sente 1.9.0-beta2 works fine, 1.10.0-SNAPSHOT failed.

Error log:
UnableToResolveError: Unable to resolve module websocket from /Users/tienson/codes/index.ios.js: Unable to find this module in its module map or any of the node_modules directories under /Users/node_modules/websocket and its parent directories.

@tiensonqin tiensonqin changed the title Unable to resolve module websocket in react native Unable to resolve module websocket in react native release mode Jul 15, 2016
@danielcompton
Copy link
Collaborator

danielcompton commented Jul 15, 2016

The node client requires the NPM websocket module to be installed as well. It's not included by default, as the dep would leak to other users who don't need it. This should probably be a bit more visible somewhere, either in the Sente README, or in https://github.com/theasp/sente-nodejs-example

@theasp
Copy link
Collaborator

theasp commented Jul 16, 2016

I've added a note to the nodejs example about requiring "websocket". I'm not sure how this impacts react native though.

@ptaoussanis
Copy link
Member

This must be due to #243.

@tiensonqin I'm not familiar with React Native, a few questions:

  • You must be building with cljs/*target* set to "nodejs", is that correct?
  • What's the relationship between React Native and node? It can use node? It must use node?
  • Any idea if React Native was working correctly over WebSockets with Sente 1.9.0-beta2?

Thanks!

@ptaoussanis
Copy link
Member

Have updated 1.10.0-SNAPSHOT. I believe this should resolve the error.

@tiensonqin 2 more questions if I may?:

  1. Could you confirm that your error's gone with the updated SNAPSHOT?
  2. Do WebSockets work with this SNAPSHOT even if you don't have the websockets npm module installed?

@tiensonqin
Copy link
Contributor Author

@ptaoussanis Sorry about the delay.

  1. I still have the error after updated to the latest commit,
    as a workaround I comment out the ?node-npm-websocket, don't have time for now, sorry!
  2. As I know it, react native don't use npm websocket, many npm modules even node core modules can't be used in react native, since they are different platform.
    react active just make websocket web browser compatible by bridgeing obj-c or java. You don't need to install npm websocket in react native.

@ptaoussanis
Copy link
Member

ptaoussanis commented Jul 18, 2016

@tiensonqin Thanks for the follow-up.


Can anyone with Node experience perhaps point me to the docs for js/require? Had assumed it'd just throw if the requested library wasn't available, but perhaps that's not the case.

EDIT For context: need this to gracefully fail if the lib's not available.

@ptaoussanis
Copy link
Member

ptaoussanis commented Jul 18, 2016

@DaveWM Alternatively: do we really need to call (js/require "websocket") ourselves? Is that a common idiom for Node? Could we not instead just check if the library's already been loaded and leave Sente users to load the lib how/where/when they want?

What exactly does the (js/require "websocket") call return when successful? Is there a websocket ns object that's created somewhere that we can just check for? Is it at a reliable/fixed location? I note that the examples at https://www.npmjs.com/package/websocket do all use this var W3CWebSocket = require('websocket').w3cwebsocket form.

Finding it remarkably difficult to locate any docs on how require is implemented or exactly what its API contract is.

@DaveWM
Copy link
Contributor

DaveWM commented Jul 18, 2016

@ptaoussanis Node's require is quite similar to clojure's require, it's documented here. It's basically just a function that takes the name of the package, and returns a module. It doesn't add anything to the global scope, so calling js/require is the only way to get the W3CWebSocket object, even if it's already been loaded.

I think the simplest fix would be to only require the websockets npm module if there isn't already a global WebSocket defined. Can submit a PR tonight if that's alright with you.

@ptaoussanis
Copy link
Member

@DaveWM Thanks for the clarification.

I think the simplest fix would be to only require the websockets npm module if there isn't already a global WebSocket defined. Can submit a PR tonight if that's alright with you.

That might work in this specific case but would prefer if we had something that's semantically equivalent to "load this library if it's available, otherwise just fail silently". Any idea how we achieve that if a try/catch doesn't seem to do it?

@DaveWM
Copy link
Contributor

DaveWM commented Jul 19, 2016

AFAIK there's no way to do that without a try/catch in node unfortunately.

Looks like the try/catch isn't working on react native because of this bug. The only workaround I can think of would be to write a macro that somehow determines whether the compile target is react native, and if it is omits the require entirely. No idea if that's possible though, I've never used react native. Anyone else have any ideas?

@ptaoussanis
Copy link
Member

@DaveWM Thanks for digging into this.

Looking into this a little more, it seems possible that this behaviour isn't even regarded as a bug. For example from: http://ruoyusun.com/2015/11/01/things-i-wish-i-were-told-about-react-native.html

You Code Does Not Run on Node.JS

The JavaScript runtime you’ve got is ether JavaScriptCore (non-debug) or V8 (debug). Even though you can use NPM and a node server is running on the background, your code does not actually run on Node.JS. So you won’t be able to use of the Node.JS packages. A typical example is jsonwebtoken, which uses NodeJS’s crypto module.

As a temporary workaround, have rearranged the WebSocket search to try js/require last; am hoping that'll avoid the issue for now.

Better ideas still very much welcome.

@tiensonqin Could you please try the latest SNAPSHOT and let us know if it's any better?

@tiensonqin
Copy link
Contributor Author

@ptaoussanis The delay and rearrange way still not work here, tested.
I think react native packager just statically analysis the bundle, find all require(module),
ignore all the runtime behavior(I also tried define ?node-npm-websocket_ as a function, not work either).

Thought about macro way suggested by @DaveWM, maybe this is the right direction.

As for #248, really busy these days, have a baby daughter, and my first mobile app Lym on beta testing, I'll try to PR in next several days.

By the way, sente works pretty strong in react native, really thank you for all the team!

@ptaoussanis
Copy link
Member

@tiensonqin Thanks for the confirmation, and congratulations on your new daughter :-)

@DaveWM So if React's really doing static analysis, really not much we can do here besides:

  1. Hope they provide a workaround
  2. As you suggested, somehow try to detect React Native and elide the require call in that case
  3. Find some other way of checking for and accessing a websocket package that's already been loaded by the user

I've no experience with Node or React Native so someone else may need to do the digging on this.

If we can't find a solution, may need to back out #243 temporarily to keep from breaking anyone with v1.10, though that seems unfortunate.

@ptaoussanis
Copy link
Member

Okay, until the React Native team make a better option available - will just need React Native users to enable the SENTE_ELIDE_JS_REQUIRE environment var at compile-time to elide any js/require calls.

@seantempesta
Copy link

How do I set that env variable? I'm using React Native and am still getting

try{return require("websocket")}catch(a){return null}

in my output file. I've tried doing this:

bash-3.2$ SENTE_ELIDE_JS_REQUIRE=TRUE && lein prod-build

And I've tried adding SENTE_ELIDE_JS_REQUIRE to the :closure-defines:.

@livtanong
Copy link

livtanong commented Apr 6, 2017

@seantempesta that refers to your command prompt.
If using bash, that would be (in bashrc)

export SENTE_ELIDE_JS_REQUIRE=true

and in fish (in fish config):

set -x SENTE_ELIDE_JS_REQUIRE true

@seantempesta
Copy link

@levitanong: Thanks. This ended up working for me.

bash-3.2$ SENTE_ELIDE_JS_REQUIRE=true lein prod-build

@currentoor
Copy link
Contributor

currentoor commented Nov 18, 2019

@ptaoussanis this System value approach makes it difficult to use Sente in a project with shadow-cljs with node and react-native targets. What if we passed an expression to js/require, then React Native's static analysis tool won't be able to detect a missing package. Something like this

(defn websocket-package-name
  "Have to do this to get around React Native's static analysis.
  https://github.com/facebook/react-native/issues/5079"
  [web]
  (str web "socket"))
(def ^:private ?node-npm-websocket_
  "nnil iff the websocket npm library[1] is available.
  Easiest way to install:
    1. Add the lein-npm[2] plugin to your `project.clj`,
    2. Add: `:npm {:dependencies [[websocket \"1.0.23\"]]}`

  [1] Ref. https://www.npmjs.com/package/websocket
  [2] Ref. https://github.com/RyanMcG/lein-npm"
  (let [package-name (websocket-package-name "web")]
    (delay ; Eager eval causes issues with React Native, Ref. #247,
      (elide-require
        (when (and node-target? (exists? js/require))
          (try
            (js/require package-name)
            ;; In particular, catch 'UnableToResolveError'
            (catch :default e
              ;; (errorf e "Unable to load npm websocket lib")
              nil)))))))

I verified this works without advanced compile. Then the try/catch and node-target? checks can actually do their job.

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

No branches or pull requests

8 participants