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

chore: Allow overriding the route filtering using a ctor param routeFilter #95

Merged
merged 7 commits into from Apr 3, 2024

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Apr 2, 2024

💡 Motivation and Context

https://twitter.com/luke_pighetti/status/1774906968886366545

💚 How did you test it?

Using a fake PosthogFlutterPlatformInterface, it was the easiest way to test it and not break or add any public APIs

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@marandaneto marandaneto marked this pull request as ready for review April 2, 2024 09:51
@marandaneto marandaneto requested a review from a team April 2, 2024 09:51
@marandaneto
Copy link
Member Author

cc @lukepighetti

@marandaneto marandaneto changed the title fix: allow tracking on PageRoute screens using the PosthogObserver as well fix: allow tracking non PageRoute screens using the PosthogObserver as well Apr 2, 2024
@lukepighetti
Copy link

lukepighetti commented Apr 2, 2024

one way to make this non-breaking for existing customers is to make isTrackableRoute a class constructor argument with a default that can be overridden

you can see that pattern here in firebase_analytics https://github.com/firebase/flutterfire/blob/master/packages/firebase_analytics/firebase_analytics/lib/observer.dart#L17-L22

i bet if you copy paste this but rename the typedef to PosthogRouteFilter folks will be able to use the same filter on firebase_analytics and posthog

you could provide the current one as a default and the new one as an alternative exported from the package

related discussion: https://twitter.com/ue_man/status/1775125806789894437

@marandaneto marandaneto changed the title fix: allow tracking non PageRoute screens using the PosthogObserver as well chore: Allow overriding the route filtering using a ctor param routeFilter Apr 2, 2024
@marandaneto
Copy link
Member Author

one way to make this non-breaking for existing customers is to make isTrackableRoute a class constructor argument with a default that can be overridden

you can see that pattern here in firebase_analytics firebase/flutterfire@master/packages/firebase_analytics/firebase_analytics/lib/observer.dart#L17-L22

i bet if you copy paste this but rename the typedef to PosthogRouteFilter folks will be able to use the same filter on firebase_analytics and posthog

you could provide the current one as a default and the new one as an alternative exported from the package

related discussion: twitter.com/ue_man/status/1775125806789894437

Makes sense, this tweet convinced me, better to allow the customization, fixes are pushed, thanks for the review.

@marandaneto marandaneto merged commit 8e3a6f3 into main Apr 3, 2024
3 checks passed
@marandaneto marandaneto deleted the fix/track-non-pageroute branch April 3, 2024 07:22
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.

None yet

3 participants