microsoft / react-native-code-push

React Native module for CodePush
http://appcenter.ms
Other
8.98k stars 1.47k forks source link

gradle calculation of nodeModulesPath is poorly prioritized #1887

Closed maxkorp closed 4 months ago

maxkorp commented 4 years ago

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

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

alexandergoncharov-zz commented 4 years ago

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!

maxkorp commented 4 years ago

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 commented 4 years ago

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

alexandergoncharov-zz commented 4 years ago

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.

maxkorp commented 4 years ago

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.

joeyfigaro commented 3 years ago

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

makirby commented 3 years ago

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.

DmitriyKirakosyan commented 4 months ago

Fixed and released.