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: Revised Usage of Remote Parameter Fetcher #18

Merged
merged 5 commits into from
Nov 12, 2023

Conversation

naipaka
Copy link
Contributor

@naipaka naipaka commented Nov 12, 2023

🙌 What I did

  • Refactor flutterfire remote parameter fetcher
    • Made RemoteParameter more user-friendly
    • Changed to execute activate() instead of fetchActivate() during real-time retrieval
    • Updated samples and tests accordingly to the above changes

✍️ What I didn't do

  • Rename package name

✅ Verification

  • Android
  • iOS
  • macOS
  • Web

Screenshots

2023-11-12.14.07.09.mov

Additional Information

Copy link

codecov bot commented Nov 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (108edd2) 47.71% compared to head (2ed9fdb) 54.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
+ Coverage   47.71%   54.40%   +6.68%     
==========================================
  Files          10       10              
  Lines         285      261      -24     
==========================================
+ Hits          136      142       +6     
+ Misses        149      119      -30     
Files Coverage Δ
...te_parameter_fetcher/lib/src/remote_parameter.dart 100.00% <100.00%> (ø)
...eter_fetcher/lib/src/remote_parameter_fetcher.dart 90.78% <100.00%> (+32.36%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

/// This is useful for when you want to force a refetch of the value.
final Future<T> Function() activateAndRefetch;
/// The subscription to the stream of updated parameter information.
final StreamSubscription<void> _subscription;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By passing subscription as an argument, it has become unnecessary for the caller of the class to be conscious of adding or removing listeners!

@visibleForTesting
Stream<RemoteConfigUpdate> get onConfigUpdated {
return _rc.onConfigUpdated;
late final Stream<RemoteConfigUpdate> onConfigUpdated = _rc.onConfigUpdated;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted in the comment, the original method format created different Streams each time it was called, resulting in only the latest Stream functioning effectively.
Therefore, we changed it to hold just one Stream per instance.
This ensures that once a Stream is created, it is continuously used for that instance, guaranteeing efficient and consistent operation.

Copy link
Member

Choose a reason for hiding this comment

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

badge
I see, I hadn't even thought about the behavior of getter and late final!

Comment on lines +117 to +120
RemoteParameter<String> getStringParameter(
String key, {
required ValueChanged<String> onConfigUpdated,
}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of using addListener, we changed the approach to pass the callback for when onConfigUpdated is triggered as an argument. This allows for a more streamlined implementation, as shown in the following example using Riverpod.

@riverpod
String stringParameter(StringParameterRef ref) {
  final fetcher = ref.watch(remoteParameterFetcherProvider);
  final parameter = fetcher.getStringParameter(
    stringParameterKey,
    onConfigUpdated: (value) => ref.state = value,
  );
  ref.onDispose(parameter.dispose);
  return Uri.parse(parameter.value);
}

@naipaka naipaka marked this pull request as ready for review November 12, 2023 05:28
@naipaka naipaka requested a review from riscait as a code owner November 12, 2023 05:28
return getString(key);
},
subscription: filteredOnConfigUpdated(key).listen((event) async {
await activate();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://firebase.google.com/docs/remote-config/get-started?platform=flutter#add-real-time-listener
The official documentation stated "Automatically fetch new parameter values" and only activate() was mentioned, so we modified it to match the documentation.

@naipaka naipaka mentioned this pull request Nov 12, 2023
4 tasks
Copy link
Member

@riscait riscait left a comment

Choose a reason for hiding this comment

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

LGTM
badge
It's great that users only need to be aware of dispose!
(Just a thought, and not directly related, but using custom Hooks would be very easy with HookWidget.)
badge
The behavior has also changed, so feat seems right instead of refactoring👍.

Comment on lines +50 to +51
Future<void> dispose() async {
await _intRemoteParameter.dispose();
Copy link
Member

Choose a reason for hiding this comment

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

badge
Thanks for the implementation!

const SizedBox(height: 16),
FilledButton(
onPressed: () async {
await _intRemoteParameter.activateAndRefetch();
Copy link
Member

Choose a reason for hiding this comment

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

badge
Does this mean that it is no longer necessary to expose methods for forced refresh such as activateAndRefetch to the outside world?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as of now, onConfigUpdate suffices, so there's no need to expose RemoteParameter externally anymore!
It seems that onConfigUpdate is also triggered when returning from the background, which was the intended use case.
In RemoteParameterFetcher, fetchAndActivate can still be used, so if re-fetching and activation are needed, we will continue to use that method.

@naipaka naipaka changed the title refactor: Refactor flutterfire remote parameter fetcher feat: Refactor flutterfire remote parameter fetcher Nov 12, 2023
@naipaka naipaka changed the title feat: Refactor flutterfire remote parameter fetcher feat: Revised Usage of Remote Parameter Fetcher Nov 12, 2023
Co-authored-by: Ryunosuke Muramatsu <[email protected]>
@naipaka naipaka merged commit e44f4d1 into main Nov 12, 2023
6 checks passed
@naipaka naipaka deleted the refactor-flutterfire-remote-parameter-fetcher branch November 12, 2023 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants