-
Notifications
You must be signed in to change notification settings - Fork 3k
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
refactor: On ui layer analytics injected by entrypoint #1472
base: main
Are you sure you want to change the base?
Conversation
@@ -62,7 +62,7 @@ fun LazyStaggeredGridScope.newsFeed( | |||
contentType = { "newsFeedItem" }, | |||
) { userNewsResource -> | |||
val context = LocalContext.current | |||
val analyticsHelper = LocalAnalyticsHelper.current | |||
val analyticsHelper = analyticsInstance(context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the Composable is recomposed, accessing and retrieving from the Hilt dependency graph can incur costs. How about optimizing this by using remember
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that reducing cost
Thank you for your opinion!
This is just a chat, but if a Composable that calls Additionally, according to the comments in the existing code, |
@yongsuk44 Thanks for your feedback
I think it is changed by the lambda code so that preview will be OK
I also agree that cost existed. |
|
@yongsuk44 I suggested CompositionLocal because I was wondering why, and I found the answer. |
Issue: #1471
Suggest changing the UI layer injection method of Analytics Instance in Analytics Module.
Instead of using CompositionLocal, how about using Hilt's EntryPoint?
It has these advantages.
Additionally, in analytics/AnalyticsModule, how about creating an instance as a singleton?