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

Prioritize nodeModulesPath over config.root if present. #2507

Conversation

joarkosberg
Copy link
Contributor

This has been discussed before in issue #1887 and in pull request #1943, but was closed because of inactivity.

Like mentioned in links, it would maybe be more natural to prioritize the direct nodeModulesPath property if present, not the reference to projects root.
And with react native 0.71 new android config, I have seen that root is present even when not setting it in project. Which means nodeModulesPath is never used, which makes it useless in that case. Therefore taking up this change again, to switch the priority.

@joarkosberg joarkosberg requested a review from a team as a code owner April 25, 2023 14:24
@DaniyarJakupov
Copy link

Faced the same issue with the wrong path to node_modules in monorepo setup after upgrading to react-native 0.72.3. Now root is always passed and it overrides custom nodeModulesPath that we have. For now used patch-package, but would be good to merge this PR
cc @DmitriyKirakosyan

@mendesr-ppb
Copy link

TL;DR

We have just upgraded to RN 0.72.3, and our project follows a monorepo setup too. While the PR isn't tested and merged, we will apply the patch as a workaround for now.

--

We were about to reply in a related thread from the Issue Tracker, but we will share our struggles here instead.

We are running RN 0.72.3 and RN CodePush 8.0.2 against a monorepo setup also with different workspaces, e.g. web and native.

We noticed that codepush.gradle can read a custom node_modules path via the nodeModulesPath project property. Inside android/app/build.gradle, we leveraged project.ext.nodeModulesPath for our convenience to tell CodePush where to find its scripts i.e. the monorepo root node_modules.

The approach worked fine while running RN 0.69.7 and CodePush 7.1.0. After the RN upgrade, we stopped seeing the project.ext.nodeModulesPath set because the react config in android/app/build.gradle would always have the root property defined with the root of the RN project, by default. Below is the error output:

Error: Cannot find module '/Users/$USER/monorepo/apps/native/node_modules/react-native-code-push/scripts/generateBundledResourcesHash.js'
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

We were hoping to read '/Users/$USER/monorepo/node_modules/react-native-code-push/scripts/generateBundledResourcesHash.js' instead.

android/app/build.gradle

/**
 * Set the monorepo root path.
 */
def rootDir = file("$projectDir/../../../../")

react {
    //   The root of your project, i.e. where "package.json" lives. Default is '..'
    // root = file("../")  <-- ´root´ will always be defined and required to point to RN's root
    reactNativeDir = file("$rootDir/node_modules/react-native")
    codegenDir = file("$rootDir/node_modules/react-native-codegen")
    cliFile = file("$rootDir/node_modules/react-native/cli.js")
    entryFile = file("$rootDir/apps/native/index.js")
    hermesCommand = "$rootDir/node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc"
}

/**
 * Code Push must use `nodeModulesPath` to assign the monorepo `node_modules`.
 */
project.ext.nodeModulesPath = "../../../../node_modules"

codepush.gradle

def nodeModulesPath;
if (config.root) {
    nodeModulesPath = Paths.get(config.root.asFile.get().absolutePath, "/node_modules");
} else if (project.hasProperty('nodeModulesPath')) {
    nodeModulesPath = project.nodeModulesPath // <-- The monorepo node modules path doesn't seem to be assigned as config.root will always be defined.
} else {
    nodeModulesPath = "../../node_modules";
}

We have attempted to override the root path in the react config to point to file("../../../../") (the monorepo root), but then the build task fails to find the metro.config file. We're happy to help if you guys need a hand.

@agusvazquez
Copy link

Hello! I am having the same issue.

Would it be possible to merge and deploy this?

@moreiraj-ppb
Copy link

Anyone to merge this?

@joarkosberg joarkosberg force-pushed the check-first-for-nodeModulesPath-property branch from 1106295 to 7078150 Compare October 10, 2023 11:04
@MikhailSuendukov
Copy link
Contributor

Could you please pull our master branch into your branch? It fixed problems with tests.

@joarkosberg joarkosberg force-pushed the check-first-for-nodeModulesPath-property branch from 7078150 to 34165ef Compare October 30, 2023 14:32
@joarkosberg
Copy link
Contributor Author

Could you please pull our master branch into your branch? It fixed problems with tests.

Fixed 🙂

@DmitriyKirakosyan DmitriyKirakosyan merged commit df2de5e into microsoft:master Nov 1, 2023
3 checks passed
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