From 5f1408858abb8d8d0b6c573bdf7b8b8f1aac0986 Mon Sep 17 00:00:00 2001 From: Giles Van Gruisen Date: Tue, 11 Jul 2017 11:04:24 -0700 Subject: [PATCH] Support additional dependency declaration format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Thanks for submitting a PR! Please read these instructions carefully: - [x] Explain the **motivation** for making this change. - [x] Provide a **test plan** demonstrating that the code is solid. - [x] Match the **code formatting** of the rest of the codebase. - [x] Target the `master` branch, NOT a "stable" branch. Previously, `isInstalled` was somewhat naively checking for the presence of a string in the `build.gradle` file to determine whether or not that dependency was already linked. I.e.: ``` compile project(':${name}')\n ``` …where `name` is replaced with the name of the dependency being checked. This was inflexible as it only supported that particular format of `compile` definition. Another, valid `compile` definition follows: ``` compile(project(':example') { … } ``` However, this failed the check because it didn't _exactly_ match the format for which the check was searching the `build.gradle` contents. As a result, running `react-native link` would incorrectly duplicate the dependency definition and thus cause a crash upon launching the app. This change adds an `installPattern` to the object returned from `makeBuildPatch`, which includes the particular dependency name and is valid for both `compile` definition formats. This commit adds an additional compile definition in the associated fixture, an additional test case in `isInstalled.spec.js` to check for this additional format, and an additional test in `makeBuildPatch.spec.js` to ensure the object returned includes the aforementioned `installPattern` Regex pattern. Sign the [CLA][2], if you haven't already. ✅ Small pull requests are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it. ✅ Make sure all **tests pass** on both [Travis][3] and [Circle CI][4]. PRs that break tests are unlikely to be merged. For more info, see the ["Pull Requests"][5] section of our "Contributing" guidelines. [1]: https://medium.com/martinkonicek/what-is-a-test-plan-8bfc840ec171#.y9lcuqqi9 [2]: https://code.facebook.com/cla [3]: https://travis-ci.org/facebook/react-native [4]: http://circleci.com/gh/facebook/react-native [5]: https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#pull-requests Closes https://github.com/facebook/react-native/pull/14475 Differential Revision: D5398552 Pulled By: hramos fbshipit-source-id: 1eaf84ba5bcfc43202f13c6b8fcfc68c30f36c33 --- local-cli/link/__fixtures__/android/patchedBuild.gradle | 3 +++ local-cli/link/__tests__/android/isInstalled.spec.js | 7 ++++--- local-cli/link/__tests__/android/makeBuildPatch.spec.js | 6 ++++++ local-cli/link/android/isInstalled.js | 5 ++--- local-cli/link/android/patches/makeBuildPatch.js | 7 ++++++- 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/local-cli/link/__fixtures__/android/patchedBuild.gradle b/local-cli/link/__fixtures__/android/patchedBuild.gradle index 8c75995d38e543..68ccfec2eaa543 100644 --- a/local-cli/link/__fixtures__/android/patchedBuild.gradle +++ b/local-cli/link/__fixtures__/android/patchedBuild.gradle @@ -1,5 +1,8 @@ dependencies { compile project(':test') + compile(project(':test2')) { + exclude(group: 'org.unwanted', module: 'test10') + } compile fileTree(dir: "libs", include: ["*.jar"]) compile "com.android.support:appcompat-v7:23.0.1" compile "com.facebook.react:react-native:0.18.+" diff --git a/local-cli/link/__tests__/android/isInstalled.spec.js b/local-cli/link/__tests__/android/isInstalled.spec.js index c85717133d5858..2d1db99a558a24 100644 --- a/local-cli/link/__tests__/android/isInstalled.spec.js +++ b/local-cli/link/__tests__/android/isInstalled.spec.js @@ -8,11 +8,12 @@ const projectConfig = { }; describe('android::isInstalled', () => { - it('should return true when project is already in build.gradle', () => + it('should return true when project is already in build.gradle', () => { expect(isInstalled(projectConfig, 'test')).toBeTruthy() - ); + expect(isInstalled(projectConfig, 'test2')).toBeTruthy() + }); it('should return false when project is not in build.gradle', () => - expect(isInstalled(projectConfig, 'test2')).toBeFalsy() + expect(isInstalled(projectConfig, 'test3')).toBeFalsy() ); }); diff --git a/local-cli/link/__tests__/android/makeBuildPatch.spec.js b/local-cli/link/__tests__/android/makeBuildPatch.spec.js index 1e3c3dcbb2a8fd..8922bbacf7b7e7 100644 --- a/local-cli/link/__tests__/android/makeBuildPatch.spec.js +++ b/local-cli/link/__tests__/android/makeBuildPatch.spec.js @@ -13,4 +13,10 @@ describe('makeBuildPatch', () => { const {patch} = makeBuildPatch(name); expect(patch).toBe(` compile project(':${name}')\n`); }); + + it('should make a correct install check pattern', () => { + const {installPattern} = makeBuildPatch(name); + const match = `/\\s{4}(compile)(\\(|\\s)(project)\\(\\':${name}\\'\\)(\\)|\\s)/`; + expect(installPattern.toString()).toBe(match); + }); }); diff --git a/local-cli/link/android/isInstalled.js b/local-cli/link/android/isInstalled.js index fda849cf59a33d..16c9194a57f926 100644 --- a/local-cli/link/android/isInstalled.js +++ b/local-cli/link/android/isInstalled.js @@ -2,7 +2,6 @@ const fs = require('fs'); const makeBuildPatch = require('./patches/makeBuildPatch'); module.exports = function isInstalled(config, name) { - return fs - .readFileSync(config.buildGradlePath) - .indexOf(makeBuildPatch(name).patch) > -1; + const buildGradle = fs.readFileSync(config.buildGradlePath); + return makeBuildPatch(name).installPattern.test(buildGradle); }; diff --git a/local-cli/link/android/patches/makeBuildPatch.js b/local-cli/link/android/patches/makeBuildPatch.js index 879acbd059b2ef..4941186a99cea5 100644 --- a/local-cli/link/android/patches/makeBuildPatch.js +++ b/local-cli/link/android/patches/makeBuildPatch.js @@ -1,6 +1,11 @@ module.exports = function makeBuildPatch(name) { + const installPattern = new RegExp( + `\\s{4}(compile)(\\(|\\s)(project)\\(\\\':${name}\\\'\\)(\\)|\\s)` + ) + return { + installPattern, pattern: /[^ \t]dependencies {\n/, - patch: ` compile project(':${name}')\n`, + patch: ` compile project(':${name}')\n` }; };