react-native-code-push icon indicating copy to clipboard operation
react-native-code-push copied to clipboard

gradle calculation of nodeModulesPath is poorly prioritized

Open maxkorp opened this issue 4 years ago • 7 comments

If you specify a project root AND nodeModulesPath in your app/build.gradle, the nodeModulesPath is ignored, and the assumed root/node_modules is used. Instead, the more specific nodeModulesPath config should be used. I have not checked if an analogous problem exists in iOS

Steps to Reproduce

  1. Set up 2 react native projects in a monorepo at monorepoRoot/packages/appA and monorepoRoot/packages/appB
  2. Fix hoisting so that react-native-code-push is installed at monorepoRoot/node_modules/react-native-code-push
  3. Set values for root and nodeModulesPath config.ext.react, e.g.
project.ext.react = [
    entryFile: "index.js",
    enableHermes: false,  // clean and rebuild if changing
    root: "../..",
    cliPath: "../../node_modules/react-native/cli.js"
]
project.ext.nodeModulesPath = "../../../../node_modules"

Expected Behavior

I would expect for the nodeModulesPath used by codepush to be ../../../../node_modules, which means the cli for bundling appA for example calls out to monorepoRoot/node_modules/react-native-code-push/scripts/generateBundledResourcesHash.js

Actual Behavior

the nodeModulesPath used by codepush ends up as ../../node_modules which means the lie for bundling appA calls out to monorepoRoot/packages/appA/node_modules/react-native-code-push/scripts/generateBundledResourcesHash.js

Proposed solution

I have not yet tested this deeply, but I unless pathing is an issue somewhere I am not seeing, we can reorder the calculation if statements to allow nodeModulesPath to take priority. Existing code (from codepush.gradle):

        def nodeModulesPath;
        if (config.root) {
            nodeModulesPath = Paths.get(config.root, "/node_modules");
        } else if (project.hasProperty('nodeModulesPath')) {
            nodeModulesPath = project.nodeModulesPath
        } else {
            nodeModulesPath = "../../node_modules";
        }

Proposed change:

        def nodeModulesPath;
        if (project.hasProperty('nodeModulesPath')) {
            nodeModulesPath = project.nodeModulesPath
        } else if (config.root) {
            nodeModulesPath = Paths.get(config.root, "/node_modules");
        } else  else {
            nodeModulesPath = "../../node_modules";
        }

Reproducible Demo

I'll work on getting one set up.

Environment

  • react-native-code-push version: 6.2.1
  • react-native version: 0.61.4
  • From inside appA for example you can run android/gradlew assembleRelease -p android/

I will continue to test more thoroughly and can submit an MR if that works. Tangentially related issue for other react-native issues around the same project structure: https://github.com/react-native-community/cli/issues/826

maxkorp avatar Jun 24 '20 21:06 maxkorp

Hi @maxkorp , Thanks for reporting and so detailed description! It is a good catch!

Please let us know results of testing your approach. And a demo app also will be helpful to investigate. Thanks!

alexandergoncharov-zz avatar Jun 25 '20 11:06 alexandergoncharov-zz

So far everything seems to work! As far as testing, I'm simply building a "release" app for android with an odd version number for a different channel, and pushing up a codepush update. I've simply edited codepush.gradle after installing my node_modules with yarn. Packaging via android/gradlew assembleRelease -p android/ works with changes but not without.

I'll get a working (not working? broken?) example repo up today for you!

maxkorp avatar Jun 25 '20 18:06 maxkorp

https://github.com/maxkorp/codepush-monorepo-example

I left populating keys to you, you can just text search for <INSERT_STAGING_KEY> and <INSERT_PRODUCTION_KEY> for both android and ios

There is a script you can run at the root, ./fix-gradle.js to patch the gradle file, and you can restore it to original with fix-gradle.js break

maxkorp avatar Jun 25 '20 22:06 maxkorp

Cool! Thanks for so many details and demo project! We will test it soon and let you know our results. We will keep you posted.

alexandergoncharov-zz avatar Jun 26 '20 10:06 alexandergoncharov-zz

Anything I can do to help out with this? I've got a bit more testing to do, but that fix script works for me locally, so I'd be happy to submit a PR with the change if you're open to it.

maxkorp avatar Sep 04 '20 20:09 maxkorp

@alexandergoncharov Friendly ping to help move this along. Issue is preventing us from releasing with codepush integrated.

joeyfigaro avatar Jan 26 '21 18:01 joeyfigaro

This worked for me just by adding the project.ext.nodeModulesPath = "../../../../node_modules" to my app/build.gradle

Can now build app in a monorepo with all packages hoisted to the root.

makirby avatar Feb 10 '21 17:02 makirby