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

Feature/v57 #192

Merged
merged 2 commits into from
Dec 3, 2018
Merged

Feature/v57 #192

merged 2 commits into from
Dec 3, 2018

Conversation

puppybits
Copy link

• Updated to react-native v0.57.0
• Upgraded lein and npm dependencies
• Added custom Metro Transformer for large Clojurescript compiled files to skip Babel transforms but still generate a sourcemap file.

@wnr wnr mentioned this pull request Sep 24, 2018

module.exports = {
transform: customClojurescriptTransformer,
projectRoot: projectRoot,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get an error here (node:19615) UnhandledPromiseRejectionWarning: ReferenceError: projectRoot is not defined

This line should probably be projectRoot: rnCliConfig.projectRoot,

@chpill
Copy link

chpill commented Sep 27, 2018

Thanks a lot for the work you are putting into this!

I'm not sure about how to use the npm run bundle-android command to build an android APK though 🤔

When do cd android && ./gradlew assembleRelease, it seems that the rn-cli.config.js file is indeed used, but it does not use the bundle-android script from the package.json.

In the end, I got it working by adding this line to my android/app/build.graddle:

 project.ext.react = [
   nodeExecutableAndArgs: ["node", "--expose-gc", "--max_old_space_size=8192"],
 ]

@chpill
Copy link

chpill commented Sep 28, 2018

I don't know if it is intentional or not, but I noticed that the bundle file that results from calling npm run bundle-android is not a production bundle (__DEV__ = true at the very top, and the file itself is not minified).

To obtain a production js bundle, I added --dev false to the options after bundle in the bundle android command. Sadly, the resulting file is broken, something bad seems to be happening to our optmimized js code 😭. @puppybits have you tried with the --dev false flag? Did the resulting bundle work for you?

Copy link
Owner

@drapanjanas drapanjanas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this contribution! 🎉 I had some small issues before it worked on my machine, but it does work. I will test it a bit more and will merge. Thanks! 👍

@@ -131,7 +131,7 @@ ensureExecutableAvailable = (executable) ->
exec "type #{executable}"

isYarnAvailable = () ->
false
exec 'yarn -v'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails if there is no yarn:

Command failed: yarn -v

Leave it as false for now?

@chpill
Copy link

chpill commented Oct 12, 2018

For the record, I finally found out the issue I had when bundling for production. It is caused by an optimization the metro bundler tries to apply to our minified/optimized main file. I've documented it here facebook/metro#291

@chpill
Copy link

chpill commented Oct 12, 2018

I have doubts about the rn-cli.config.js file. From what I have seen on my project, the custom transforms defined there do not appear to be taken into account at all by metro.

The fact that it solves the memory issue seems to rely solely on the manual garbage collection (that does run).

I may have found a cleaner way: Babel has a way to declare that some files should simply be ignored. So if you use something like .re-natal, you can add this to your .babelrc

  "ignore": [
    "./index.ios.js",
    "./index.android.js"
  ]
}

This used to break the metro bundler, so you could not use it, but it has been fixed very recently through facebook/metro@66aa795

I have applied the patch locally on my node_modules, and I haven't run into memory issues since. Hopefully it will be available through the next release of react-native, or maybe someone knows how to force react-native to use more recent version of metro?

@codonovan
Copy link

codonovan commented Oct 12, 2018

@chpill react-native 0.57.3 has just been released and is using metro 0.48.1

I had to modify rn-cli.config.js slightly to get it working with newer versions of metro.

// @see https://facebook.github.io/metro/docs/en/configuration
var rnCliConfig = {
  projectRoot : path.resolve(__dirname),
  transform   : customClojurescriptTransformer
};

module.exports = {
  transform: customClojurescriptTransformer,
  projectRoot: rnCliConfig.projectRoot,
  transformer: {
    babelTransformerPath: require.resolve("./rn-cli.config.js")
  }
};

@drapanjanas
Copy link
Owner

@chpill custom transformer indeed did not work before. The fix by @codonovan makes it work, but so far, at least on my MacBook I can't get it running.
screen shot 2018-10-21 at 21 23 53
I probably will create a new branch merge this PR there with all additional changes in there and will only merge it in master once it really works well.

@codonovan
Copy link

codonovan commented Oct 21, 2018

@drapanjanas I think that error is due to the regex at line 84 in rn-cli.config.js The first line in customClojurescriptTransformer. I changed it to

var customClojurescriptTransformer = function(data) {
  if(data.filename === 'index.android.js' || data.filename === 'index.ios.js') {
    return clojurescriptTransformer(data);
  } else {
    return defaultTransformer.transform(data);
  }
};

@chpill
Copy link

chpill commented Oct 22, 2018

@codonovan @puppybits just to be clear, in the end I did not use any rn-cli.config.js file to get my build working. I used a custom patch to get the change in metro (which is now released), and I simply added the "ignore" entry to my .babelrc file

@xfyre
Copy link

xfyre commented Oct 24, 2018

Is this update supposed to fix figwheel live reloading? It appears to be broken after update to RN 0.57.2 as well. I'm trying to figure out how to get it back.

@vinurs
Copy link

vinurs commented Oct 28, 2018

@codonovan i changed the code by your sugesstion, but the problem still here:
image

@drapanjanas
Copy link
Owner

drapanjanas commented Oct 28, 2018

@chpill thanks! I got the build working now without using cutom transformer. But now I have the problem figwheel does not reload files. REPL works fine, so I think it is not a general problem with connection. Do you experience that?

Looks like :jsload-callback is not getting called as pointed out by @xfyre

@chpill
Copy link

chpill commented Oct 29, 2018

@drapanjanas I'm not experiencing issues with hot-reloading using RN 57.0... That said I somewhat diverged from re-natal a while ago (I needed a single cljs build for both ios and android). So I miss a few improvements from the latest version, mainly: I am not yet using :target :nodejs.

But I see that the other PR by @wnr includes a change in the fighweel-bridge file that may be relevant to you https://github.com/drapanjanas/re-natal/pull/193/files#diff-a27f34062e9e57b68e6346717e0db9e5.

@olivergeorge
Copy link

olivergeorge commented Oct 30, 2018

@drapanjanas - On slack @xfyre suggested "you shouldn’t upgrade Figwheel to a version older than 0.5.14 (you’ll lose live reloading)".

I think this is the related figwheel issue bhauman/lein-figwheel#716 but @xfyre is in a better position to comment.

@drapanjanas drapanjanas merged commit 649c439 into drapanjanas:master Dec 3, 2018
@gtebbutt
Copy link

gtebbutt commented Dec 7, 2018

@drapanjanas Great to see this merged! Is there any chance of a new npm release to include 0.57 support?

@drapanjanas
Copy link
Owner

@gtebbutt new npm version is published 😉

@vinurs
Copy link

vinurs commented Dec 7, 2018

@drapanjanas we have waitted it for a long time, thanks a lot

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.

8 participants