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: enable dependency injection for CustomerIO #85

Closed

Conversation

AristideVB
Copy link

Description:

This PR refactors the CustomerIO class to improve its testability, flexibility, and ease of use with Dependency Injection. The key changes made are converting the static methods to instance methods and introducing a singleton instance of CustomerIO for ease of use.

Changes:

  1. Convert static methods in CustomerIO to instance methods.
  2. Introduce a singleton instance to maintain ease of use in existing implementations.

Example:

Before:

// Using static method
CustomerIO.subscribeToInAppEventListener(_handleInAppEvent);

After:

// Using instance method
final customerIO = CustomerIO.instance;
customerIO.subscribeToInAppEventListener(_handleInAppEvent);

Practical Usage in a Bloc:

class InAppMessagesBloc extends Bloc<InAppMessagesEvent, InAppMessagesState> {
  InAppMessagesBloc(CustomerIO customerIO) : super(InAppMessagesInitial()) {
    on<InAppMessagesEventReceived>(_onInAppMessagesEventReceived);

    _inAppMessageStreamSubscription =
        customerIO.subscribeToInAppEventListener(_handleInAppEvent);
  }

  late StreamSubscription<dynamic> _inAppMessageStreamSubscription;
}

Benefits:

  • Enables Dependency Injection, making the class more modular and testable.
  • Preserves ease of use by providing a singleton instance for straightforward migration paths.
  • Allows for more flexibility in various usage scenarios.

Additional Information:

This change aligns with the modern best practices of Flutter development and makes the package more robust for complex applications.

Acceptance Criteria:

  • All static methods are converted to instance methods.
  • Singleton instance is provided for backward compatibility.
  • Unit tests are added to verify that the changes meet the desired outcomes.

This PR aims to address Issue #84.

Copy link
Collaborator

@Shahroz16 Shahroz16 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @AristideVB, it indeed adds flexibility. But this is a breaking change as it would break the current implementation of the folks, so I would rather suggest deprecating the old methods and adding these new ones and then eventually removing the old methods later on.

@AristideVB
Copy link
Author

AristideVB commented Sep 25, 2023

Thanks for the review, @Shahroz16. I get your point about avoiding breaking changes.

However, I believe a version bump would be the most efficient approach here. It not only allows to maintain a cleaner and more intuitive API but also avoids the issue of naming conflicts.

Which would be inevitable if we keep both the old and new methods.

  @deprecated
  static Future<void> initialize({
    required CustomerIOConfig config,
  }) {
    return _customerIO.initialize(config: config);
  }

  Future<void> initialize({
    required CustomerIOConfig config,
  }) {
    return _customerIO.initialize(config: config);
  }

To make things smoother, I can provide a detailed migration guide for existing users. Let me know what you think.

@Shahroz16
Copy link
Collaborator

@AristideVB Thank you again for your PR. I've shared it with the team and product, and we'll be reviewing it soon. I'll keep you updated on our next steps once I hear more.

@Shahroz16
Copy link
Collaborator

The PR was very valuable, thank you for creating it. They will be bundled up with some of the other changes in the pipeline, that are going to affect the public-facing APIs.

@Shahroz16 Shahroz16 closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants