Skip to content

Commit

Permalink
fix: topNative name changed to topNativeEvent.
Browse files Browse the repository at this point in the history
  • Loading branch information
@ugurdalkiran @dylancom
ugurdalkiran authored and dylancom committed May 19, 2024
1 parent 4ad8bc7 commit b4cbea3
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion android/src/main/java/io/invertase/googlemobileads/OnNativeEvent.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ class OnNativeEvent(viewId: Int, private val event: WritableMap) : Event<OnNativ
}

companion object {
const val EVENT_NAME = "topNative"
const val EVENT_NAME = "topNativeEvent"
}
}

5 comments on commit b4cbea3

@birdofpreyru
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @ugurdalkiran , @dylancom , may I wonder why did you change this event name? Because I've encountered an obscure bug with [email protected] on Android, New Arch, bridgeless facebook/react-native#44555, and narrowed it down to seemingly being caused by react-native-google-mobile-ads, and being fixed by this name change. I'd like to understand why it makes the difference (or may be I totally misunderstood and mis-troubleshoot my issue); and if it is in fact was causing problems to host RN app, perhaps to ask RN team if such problems can be auto-detected and warned about (or crash the app), because in my case the issue was not apparent at all.

@ugurdalkiran
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@birdofpreyru @dylancom

Since I'm working on a completely new architecture now, I solved the problem by changing it this way. But I understand that it should be "topNative" in the default structure.

I don't know how we should proceed.

@birdofpreyru
Copy link
Contributor

Choose a reason for hiding this comment

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

@ugurdalkiran i meant “topNativeEvent” works for me and solves the problem, I’m just not understanding and asking why :)

@philIip
Copy link

Choose a reason for hiding this comment

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

hi @ugurdalkiran i'm from the react native core team! i'm curious what the original issue was that you fixed and how you arrived to this solution? just wanted to figure this out to see if there's an issue we need to fix in the core infrastructure!

@philIip
Copy link

@philIip philIip commented on b4cbea3 Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hi @ugurdalkiran i'm from the react native core team! i'm curious what the original issue was that you fixed and how you arrived to this solution? just wanted to figure this out to see if there's an issue we need to fix in the core infrastructure!

thanks to @dmytrorykun i understand why this change needed to be made. essentially from very very legacy stuff, we have two different events types - events that go into the react event system, and events that get dispatched as a result of those events. in practice these names ended up being the same (i.e., i dispatch a event string to react, it will dispatch the same event string out). however, in the legacy android architecture, we still allowed these event strings to be different (i.e., dispatch "topNative" to React, which would then dispatch "topNativeEvent"), which is what caused this to redbox.

cc @birdofpreyru @cortinico

Please sign in to comment.