Skip to content

Commit

Permalink
Support additional dependency declaration format
Browse files Browse the repository at this point in the history
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 #14475

Differential Revision: D5398552

Pulled By: hramos

fbshipit-source-id: 1eaf84ba5bcfc43202f13c6b8fcfc68c30f36c33
  • Loading branch information
gilesvangruisen authored and facebook-github-bot committed Jul 11, 2017
1 parent 51e258e commit 5f14088
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 7 deletions.
3 changes: 3 additions & 0 deletions local-cli/link/__fixtures__/android/patchedBuild.gradle
Original file line number Diff line number Diff line change
@@ -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.+"
Expand Down
7 changes: 4 additions & 3 deletions local-cli/link/__tests__/android/isInstalled.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
);
});
6 changes: 6 additions & 0 deletions local-cli/link/__tests__/android/makeBuildPatch.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
5 changes: 2 additions & 3 deletions local-cli/link/android/isInstalled.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
7 changes: 6 additions & 1 deletion local-cli/link/android/patches/makeBuildPatch.js
Original file line number Diff line number Diff line change
@@ -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`
};
};

0 comments on commit 5f14088

Please sign in to comment.