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

[Android] Support additional dependency declaration format #14475

Conversation

gilesvangruisen
Copy link
Contributor

@gilesvangruisen gilesvangruisen commented Jun 13, 2017

Thanks for submitting a PR! Please read these instructions carefully:

  • Explain the motivation for making this change.
  • Provide a test plan demonstrating that the code is solid.
  • Match the code formatting of the rest of the codebase.
  • Target the master branch, NOT a "stable" branch.

Motivation (required)

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.

Test Plan (required)

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.

Next Steps

Sign the CLA, 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 and Circle CI. PRs that break tests are unlikely to be merged.

For more info, see the "Pull Requests" section of our "Contributing" guidelines.

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.
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jun 13, 2017
@gilesvangruisen gilesvangruisen force-pushed the gvg-makeBuildPatch-patch branch from 783518c to dda1374 Compare June 13, 2017 00:13
@gilesvangruisen
Copy link
Contributor Author

I can't seem to figure out why these Travis CI builds are failing. Any insight?

@gilesvangruisen
Copy link
Contributor Author

bump

Anyone available to take a look and (hopefully) help me get this merged? Thanks!

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 11, 2017
@facebook-github-bot
Copy link
Contributor

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
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 facebook/react-native#14475

Differential Revision: D5398552

Pulled By: hramos

fbshipit-source-id: 1eaf84ba5bcfc43202f13c6b8fcfc68c30f36c33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants