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

Cleanup Android dependencies #77

Merged
merged 9 commits into from
Aug 31, 2022
Merged

Cleanup Android dependencies #77

merged 9 commits into from
Aug 31, 2022

Conversation

sugarmanz
Copy link
Member

No description provided.

@sugarmanz sugarmanz enabled auto-merge (squash) August 31, 2022 01:57
@sugarmanz sugarmanz force-pushed the cleanup-android-deps branch from 8dfbaab to 4f87612 Compare August 31, 2022 17:53
@sugarmanz sugarmanz merged commit 9ecffb2 into main Aug 31, 2022
@sugarmanz sugarmanz deleted the cleanup-android-deps branch August 31, 2022 18:17
),
transition = "1.4.1",
),
dagger = "2.35.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

i recall you saying you needed to add this for some odd reason, do we still need this

Copy link
Member Author

Choose a reason for hiding this comment

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

dagger is needed to load the grab common repository -- at least it was when i initially integrated. it'd be cool to get off that repo eventually

@@ -13,4 +13,4 @@ load("@grab_bazel_common//:workspace_defs.bzl", grab = "GRAB_BAZEL_COMMON_ARTIFA

tooling = distribution + grab

maven = common + core + graaljs + j2v8 + utils + testutils + perf + plugins + tooling + android + demo
maven = remove_duplicates(common + core + graaljs + j2v8 + utils + testutils + perf + plugins + tooling + android)
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥


main_exports = [
"//android/player",
]

main_deps = main_exports + parse_coordinates(maven) + [
main_deps = main_exports + parse_coordinates(maven_main) + [
Copy link
Contributor

Choose a reason for hiding this comment

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

since maven_main doesn't have anything in it right now, maybe remove the parse_coordinates, i'm not against keeping the maven_main though

Copy link
Member Author

@sugarmanz sugarmanz Aug 31, 2022

Choose a reason for hiding this comment

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

This is more for the patternization of deps.bzl. If someone wants to add another dependency, all they need to do right now is add "androidx.core:core-ktx:%s" % versions.androidx.core" and that will automatically be pulled into the maven_install_and_ converted to the corresponding@maven//:androidx_core_core_ktx` Bazel reference.

If we remove the parse_coordinates, then they'd need to do:

converted to the corresponding @maven//:androidx_core_core_ktx Bazel reference.

manually, or reintroduce the parse_coordinates method.

"androidx.lifecycle:lifecycle-runtime-ktx:%s" % versions.androidx.lifecycle,
"androidx.lifecycle:lifecycle-viewmodel-ktx:%s" % versions.androidx.lifecycle,

# Default fallback
Copy link
Contributor

Choose a reason for hiding this comment

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

what's constraintlayout gotta do with default fallback? you mean the fallbackView? is that the only place we use it..?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love to not have a dependency on constraintlayout in the base android player dep, but we are using it to build the default fallback view:

https://github.com/player-ui/player/blob/main/android/player/src/main/res/layout/default_fallback.xml#L2

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.

2 participants