Skip to content

Apply react.gradle from node_modules/react-native #6610

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Apply react.gradle from node_modules/react-native #6610

wants to merge 2 commits into from

Conversation

satya164
Copy link
Contributor

The goal is to minimize the number of files we need to bootstrap. This allows us to make the upgrade process smoother for everyone.

If someone needs to customize the file, we already provide some config options. The ability to copy the file and modify it is always there for those few who need it.

Test plan

Generate a new project with the updated template. The app should build and run fine both in debug and production mode.

Related #6292

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @foghina, @cpojer and @mkonicek to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Mar 23, 2016
@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

@satya164
Copy link
Contributor Author

cc @mkonicek @foghina @bestander

foghina
foghina reviewed Mar 29, 2016
View reviewed changes
react.gradle
@@ -72,10 +72,10 @@ gradle.projectsEvaluated {
// Set up dev mode
def devEnabled = !targetName.toLowerCase().contains("release")
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
commandLine "cmd", "/c", "node", "node_modules/react-native/local-cli/cli.js", "bundle", "--platform", "android", "--dev", "${devEnabled}",
commandLine "cmd", "/c", "react-native", "bundle", "--platform", "android", "--dev", "${devEnabled}",
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes look unrelated.

@foghina
Copy link
Contributor

foghina commented Mar 29, 2016

Also, perhaps the top level is not the best place react.gradle? I think it would be better in ReactAndroid/, but if that interferes with the build in any way I'm fine with top level.

@satya164
Apply react.gradle from node_modules/react-native
0b26644 
The goal is to minimize the number of files we need to bootstrap. This allows us to make the upgrade process smoother for everyone.
@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

@satya164
Copy link
Contributor Author

@foghina We can move it to ReactAndroid, but IMHO we shouldn't be shipping ReactAndroid to NPM. Adds 5 MB of extra download for no good reason.

@foghina
Copy link
Contributor

foghina commented Mar 29, 2016

we shouldn't be shipping ReactAndroid to NPM

Argh, forgot about that, good point. Looks good, thanks!

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@bestander
Copy link
Contributor

Looks like tests got broken, @satya164

@satya164
Copy link
Contributor Author

@bestander Checking

@satya164
Copy link
Contributor Author

@bestander Seem to be unrelated to this change? They are regarding adb issues, like unable to connect device.

@bestander
Copy link
Contributor

There are 2 issues:

  1. npm bundle test
  2. adb gradle install

It is odd that both failed consistently 2 times https://circleci.com/gh/facebook/react-native/tree/pull%2F6610
I'll try rebuilding without caches

@satya164
Copy link
Contributor Author

@bestander Weird. I'm unable to find the cause from the logs. Still trying to figure out why it's failing.

@bestander
Copy link
Contributor

It passes on Travis which is Node 4 and npm 2.
Maybe it is npm 3 thing?

@satya164
Copy link
Contributor Author

@bestander Weird, why would NPM version affect moving a gradle file!!!

@bestander
Copy link
Contributor

That is completely mad, that is true.
Maybe bloody packager decided to read gradle? :)

@satya164
Copy link
Contributor Author

@bestander Maybe because I didn't merge master to my branch?

@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

@bestander
Copy link
Contributor

Try rebasing on master, @satya164.
And don't bother squashing commits, having many would help us track when changes introduce CI failures.

     // get entry file complete path (`entryFile` is relative to roots)
      let entryFilePath;
      if (response.dependencies.length > 1) { // skip HMR requests
        const numModuleSystemDependencies =
          this._resolver.getModuleSystemDependencies({dev, unbundle}).length;

        entryFilePath = response.dependencies[
          (response.numPrependedDependencies || 0) +
          numModuleSystemDependencies
        ].path;
      }

@satya164
Copy link
Contributor Author

@bestander Rebased. Fingers crossed!

@bestander
Copy link
Contributor

Ok, tests now pass

@bestander
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in a7cde80 Mar 29, 2016
@satya164 satya164 deleted the react-gradle branch March 29, 2016 15:30
@foghina
Copy link
Contributor

foghina commented Mar 29, 2016

Thanks for picking this up @bestander!

@ColCh ColCh mentioned this pull request May 4, 2016
Closed
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
@satya164 @zebulgar
Apply react.gradle from node_modules/react-native
7f97b0e 
Summary:The goal is to minimize the number of files we need to bootstrap. This allows us to make the upgrade process smoother for everyone.

If someone needs to customize the file, we already provide some config options. The ability to copy the file and modify it is always there for those few who need it.

**Test plan**

Generate a new project with the updated template.  The app should build and run fine both in debug and production mode.

Related facebook#6292
Closes facebook#6610

Differential Revision: D3109099

Pulled By: foghina

fb-gh-sync-id: 13fc89e60daed30bf6349e532a140c1b6f8f053a
fbshipit-source-id: 13fc89e60daed30bf6349e532a140c1b6f8f053a
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
@satya164
Apply react.gradle from node_modules/react-native
7480d35 
Summary:The goal is to minimize the number of files we need to bootstrap. This allows us to make the upgrade process smoother for everyone.

If someone needs to customize the file, we already provide some config options. The ability to copy the file and modify it is always there for those few who need it.

**Test plan**

Generate a new project with the updated template.  The app should build and run fine both in debug and production mode.

Related #6292
Closes facebook/react-native#6610

Differential Revision: D3109099

Pulled By: foghina

fb-gh-sync-id: 13fc89e60daed30bf6349e532a140c1b6f8f053a
fbshipit-source-id: 13fc89e60daed30bf6349e532a140c1b6f8f053a
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@satya164 @facebook-github-bot @foghina @bestander