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

feat(android): simple installation #414

Merged
merged 5 commits into from
May 29, 2022
Merged

feat(android): simple installation #414

merged 5 commits into from
May 29, 2022

Conversation

bang9
Copy link
Contributor

@bang9 bang9 commented May 25, 2022

Hello Team, thank you for always supporting high-quality open source!
I got the idea from react-native-gradle and simplifies the installation.
It works, but I'm not sure of any side effects because I don't have deep knowledge of groovy.
so feel free to comment. thank you!

@helenaford
Copy link
Member

ooh thanks for this p/r 😍 it looks like it should work 👀 I'll double-check locally, and I'll kick off the jobs.

Would you be able to sign the cla?

Thank you!

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

The CI failures seem both real and unrelated to this PR

  • jest is failing and has been for a few builds, looks like ts-jest module is missing somehow
  • iOS build is failing because Podfile and Podfile.lock are out of sync

Not sure when this happens but it looks like an incomplete dependency update happened at some point. That's usually me doing dependency updates so perhaps I made a mistake along the road somewhere. Either way this will definitely simplify the android installation if it works, curious for @helenaford testing results, at the same time, I think this PR would be most complete if the tests_react_native app removed the repository definition in order for this to actually be exercised in CI -

maven {
url "$rootDir/../node_modules/@notifee/react-native/android/libs"
}

as well as the example

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Nice!

The CI failures seem both real and unrelated to this PR

  • jest is failing and has been for a few builds, looks like ts-jest module is missing somehow
  • iOS build is failing because Podfile and Podfile.lock are out of sync

Not sure when this happens but it looks like an incomplete dependency update happened at some point. That's usually me doing dependency updates so perhaps I made a mistake along the road somewhere. Either way this will definitely simplify the android installation if it works, curious for @helenaford testing results, at the same time, I think this PR would be most complete if the tests_react_native app removed the repository definition in order for this to actually be exercised in CI -

maven {
url "$rootDir/../node_modules/@notifee/react-native/android/libs"
}

as well as the example

maven { url "$rootDir/../node_modules/@notifee/react-native/android/libs" }

and the example construction script

sed -i -e $'s/mavenLocal()/mavenLocal()\\\n maven \{ url "$rootDir\/..\/node_modules\/@notifee\/react-native\/android\/libs" \}/' android/build.gradle
rm -f android/build.gradle??

That will purge it out of everywhere and then CI will demonstrate whether it works or not I think?

Comment on lines 13 to +14
"dependencies": {
"@notifee/react-native": "github:notifee/react-native-notifee",
"@notifee/react-native": "link:../",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I linked to the current project to check the build with changes, please let me know if a revert is needed!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this was there for some reason, but in retrospect I can't remember what it is. I think the link to local is better personally and I'd like it to work. Willing to go with it and see if it works, personally

@bang9 bang9 requested a review from mikehardy May 25, 2022 13:53
@@ -24,15 +23,5 @@ const setCompileSdkVersion = (buildGradle: string): string => {
return buildGradle.replace(/compileSdkVersion = 30/, `compileSdkVersion = 31`);
};

const setMavenRepository = (projectBuildGradle: string): string => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh you got the expo plugin too, I forgot about it 🤦 - nicely done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd never know if you hadn't told me about the tests. 👍

@mikehardy
Copy link
Collaborator

mikehardy commented May 25, 2022

Believe it or not, the CI e2e android test is encouraging - it did fail, but it did build, failure is just a flaky test.
It would (should?) fail completely if the repo addition in the build.gradle was unable to locate the generated AAR
This is a big enough change that it still makes sense to pull it locally, and I have restarted the android e2e ci run (it does pass sometimes, though it has a flaky test more frequently then I wish), but this appears to be working!

@bang9
Copy link
Contributor Author

bang9 commented May 26, 2022

I attached a jest result screenshot ran from my local !
image

If there is a high rate that tests cannot be performed because the ts-jest path cannot be found, we may consider specifying the path directly💡
image

@helenaford helenaford merged commit 379d0a6 into invertase:main May 29, 2022
@helenaford
Copy link
Member

thank you!! Merging! And about to release 😎

@bang9
Copy link
Contributor Author

bang9 commented May 30, 2022

@mikehardy @helenaford Thank you!

@bang9 bang9 deleted the patch-1 branch May 31, 2022 17:58
@@ -87,3 +87,10 @@ ReactNative.shared.applyPackageVersion()
ReactNative.shared.applyDefaultExcludes()
ReactNative.module.applyAndroidVersions()
ReactNative.module.applyReactNativeDependency("api")
rootProject.allprojects {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@christocracy - you might like this, removes an error-prone install step for react-native repos with local maven repositories to load aars, appears to work fine for us here, could clean up my app build files from background-fetch and geolocation-android :-)

Choose a reason for hiding this comment

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

Whoa, cool!!

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.

4 participants