mjmeintjes / boot-react-native

Better development experience with ClojureScript and React Native
117 stars 14 forks source link

Updating to RN 0.26.2 #49

Closed pesterhazy closed 8 years ago

pesterhazy commented 8 years ago

I'm in the process of updating to the latest react-native, 0.26.2. However, I'm running into issues, probably related to changes in the react native packager (again).

Here's what I see in our app:

Shimming window.location
goog.require could not find: cljs.core
Error while loading cljs.core
 { [Error: goog.require could not find: cljs.core]
      line: 78203,
      column: 16,
      sourceURL: 'http://localhost:8081/index.ios.bundle?platform=ios&dev=true&hot=true' }

My guess is that the latest changes broke the BRN shims that enable goog.require to work with the RN packager's require fn.

pesterhazy commented 8 years ago

Looks like the packager really can't find the module (although brn puts it in the right place, in node_modules). It looks like the "module ids" have changed (they are now integers rather than string).

Also the packager was completely rewritten, with package loading implemented in C++, so this looks like the likely reason.

pesterhazy commented 8 years ago

After some digging, I think I've identified the problem, i.e. why interactive (online bundle) loading doesn't work anymore. The cljs compiler (optimizations none) produces require statements of the form

goog.require("core.cljs");

The 0.19 RN packager goes in and scans all input files for a string like require(...) and replaces the static string with the module id. With 0.19, this also applies to goog.require calls (because the regex applies to them). The result something like is:

goog.require('MyApp/build/node_modules/cljs.core.js');

The string is the "canonical module id" and doesn't necessarily correspond to an actual path because the first component is made up.

This doesn't seem to work with RN packager version >= 0.23 anymore, perhaps because the goog.require statements aren't identified as RN requires anymore.

pesterhazy commented 8 years ago

The relevant change is https://github.com/facebook/react-native/commit/fa3a67e251fe235af511301f893b41df1444e808. Since that commit, node-haste handles extracting dependencies (finding requires). I guess node-haste is smarter than its predecessor and doesn't parse goog.require as require.

mjmeintjes commented 8 years ago

Thanks for the investigation.

That's strange, because looking at the code at https://github.com/facebook/node-haste/blob/master/src/lib/extractRequires.js and https://github.com/facebook/node-haste/blob/master/src/lib/replacePatterns.js it should still pick up goog.require.

I'll do some investigation of my own, as I am planning to update the tool to the latest version of React Native.

pesterhazy commented 8 years ago

@mjmeintjes, I saw that as well. I added a few console.log's in extractRequires and it never got called by the packager. So unless I missed something (which is def possible), require tracking must have been replaced by some better kind of "static analysis". Though I haven't found the place where that happens either :(

pesterhazy commented 8 years ago

Worked on this a bit today. My suspicion was right, goog.require is not considered a require anymore by recent packagers (I tried 0.27.2).

I could make it work somewhat by regex-replacing goog.require calls to:

goog.require('cljs.core');  /* hackity hack */ !function(){require('cljs.core');};

That'll trick the packager into recognizing cljs.core as a dependency. However, the app still doesn't find cljs.core; it says "goog.require could not find cljs.core. Probably shimming doesn't work properly.

What's more, the packager seems to have become a lot slower; simply walking the dependency tree takes 20s or more on my laptop.

pesterhazy commented 8 years ago

I also did two more things:

        try {
            console.log("Trying RN require", name);
            require(name);
        } catch (e) {
            console.log("failed with", e);
            try {
                var mname = "brnlive/node_modules/" + name + ".js";
                console.log("Trying mangled RN require", mname);
                require(mname);
            }
            catch (e) {
                console.log("failed with", e);
                console.log("Trying goog.require", name);
                //Try default goog.require behaviour
                var oldDebugLoader = goog.ENABLE_DEBUG_LOADER;
                goog.ENABLE_DEBUG_LOADER = true;
                try {
                    orig_require.call(goog, name);
                } catch (e) {
                    console.warn("Error while loading " + name);
                    console.error(e);
                } finally {
                    goog.ENABLE_DEBUG_LOADER = oldDebugLoader;
                }
            }
        } finally {
            global.ErrorUtils.reportFatalError = oldErrorReporter; 
        }

Got me further but still didn't succeed in getting the app to run.

Also I think I had to add react-dom as an explicit dependency in package.json.

mjmeintjes commented 8 years ago

I've been going down a similar route. Do you have any more info on the errors you are getting when doing this?

vikeri commented 8 years ago

Any progress on this? I would very much like to switch from re-natal if one can have full stack traces with sourcemaps (the file and line in cljs where the error was thrown). But I'm currently on 0.28 so I can't switch if only 0.26 is supported. I haven't used boot too much but if there would be any way of helping out I'd be happy to do so.

boorad commented 8 years ago

Okay, I haven't been this deep in javascript in years, and things have changed a bit... Here's what I found:

In more recent versions of react-native, like 0.29.0, I've traced this issue to node-haste 2.12.0 here: https://github.com/facebook/node-haste/blob/master/src/Module.js#L149-L159

That shows up in node_modules code as:

          return codePromise.then(function (result) {
            var code = result.code;
            var _result$dependencies = result.dependencies;
            var dependencies = _result$dependencies === undefined ? extern ? [] : _this4._extractor(code).deps.sync : _result$dependencies;
            if (_this4._options && _this4._options.cacheTransformResults === false) {
              return { dependencies: dependencies };
            } else {
              return _extends({}, result, { dependencies: dependencies, id: id, source: source });
            }
          });

where best I can tell, result.dependencies or _result$dependencies is always [], not undefined, so the extractor, extractRequires, is never called.

Does this happen due to our transformer, cljs-rn-transformer.js? I believe it's being called here: https://github.com/facebook/node-haste/blob/master/src/Module.js#L145-L148. I can't find what sets the dependencies to [].

I have been starting my packager like so:

node_modules/react-native/packager/packager.sh --verbose true --transformer cljs-rn-transformer.js --isolateModuleIDs true --reset-cache

No idea if the --isolateModuleIDs true does anything, but --reset-cache has helped a bit. The source of those two suggestions is lost in a sea of Chrome tabs.

Hopefully this helps us move forward a bit. Like @vikeri, I'm eager to develop in this boot setup, vs. re-natal. And now, I'm fairly intimate with the packager/bundler and node-haste code, so if someone can do a bit more research, or if this jogs some memories, I'll still be around to help.

pesterhazy commented 8 years ago

Thanks @boorad for looking into this!

I made progress, albeit with a lot of crutches. I updated my "develop" branch (https://github.com/pesterhazy/boot-react-native/tree/develop) accordingly. The app now opens in the iOS simulator using RN 0.27.2.

Three more steps were needed:

The RN packager takes a long time to parse the deps, but eventually it starts.

This makes the app start, but there are obvious questions about this approach:

Another concern is with the packager's speed, which has degraded significantly (I'm guessing because it now actually parses the JS files). Though possibly this is a one-time penalty, so that subsequent updates are faster.

pesterhazy commented 8 years ago

Another approach would be to use cljs-rn-transformer.js, but I haven't figured out what transformers actually do. Can they only add source maps, or can they be used to apply the moral equivalent of s/require/goog.require/ to all cljs output?

Forgot to mention, it's useful to run the packager with export 'DEBUG=ReactNativePackager:*' to see which files it.

pesterhazy commented 8 years ago

@boorad, interesting analysis.

I've tried to figure out why the code fragment goog.require('cljs.core'); is not recognized as a dep anymore. Looking at REQUIRE_RE, this same regex still matches this string! So if extractRequires is still used, I don't understand why it doesn't match goog.require (and does match the line if I add my hackity-hack as per https://github.com/mjmeintjes/boot-react-native/issues/49#issuecomment-226569142.

Is your assumption that node-haste assumes the cljs generated files to be "extern", i.e. JSON-like files?

pesterhazy commented 8 years ago

With this minimal example:

bash-3.2$ cat hello.js
goog.require('./goodbye.js');
require('./aurevoir.js');
bash-3.2$ cat goodbye.js
console.log("goodbye");
bash-3.2$ cat aurevoir.js
console.log("au revoir");

invoking the packager like so

app/node_modules/react-native/packager/packager.sh --transformer app/build/transformer/cljs-rn-transformer.js --root app,app/build --debug --nonPersistent --resetCache

and logging out result.dependencies, I get "aurevoir.js" as a dependency, but not "goodbye.js". So there must be some difference between the two

pesterhazy commented 8 years ago

... and it also doesn't work if I remove the transformer from the commandline

pesterhazy commented 8 years ago

OK so the code in question is actually in react-native, not node-haste: https://github.com/facebook/react-native/blob/master/packager/react-packager/src/JSTransformer/worker/extract-dependencies.js#L38

It should be easy to patch this to allow goog.require as well

pesterhazy commented 8 years ago

Made a patch in my fork of react-native (based off v0.29.2): https://github.com/pesterhazy/react-native/tree/goog-require

pesterhazy commented 8 years ago

Updated my develop branch to use the patched version of react-native. https://github.com/pesterhazy/boot-react-native/tree/develop -- seems to work fine!

pesterhazy commented 8 years ago

Fixed reloading as well on my develop branch

pesterhazy commented 8 years ago

fixed in master