-
Notifications
You must be signed in to change notification settings - Fork 88
refactor: Hides configuration from public interface and make isBuilt final #233
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
base: main
Are you sure you want to change the base?
refactor: Hides configuration from public interface and make isBuilt final #233
Conversation
- The purpose of these changes is because having both configuration and isBuilt publicly mutable is misleading, since changing those attributes will have no impact.
cbf9f97
to
aa863e9
Compare
This removes a field from the public interface, which means it is a breaking change. So my question is if this should create a new major release or not. |
Hi @gabrielgarciagava, thanks for bringing this up. It makes sense to me that it's kind of misleading. We will make |
Of course. But please consider hiding it in a next major release. Or at least making the field final, such as all configuration fields as well. This way a user of the library will never be able to modify the configuration anymore after amplitude is initialized, which makes sense since it is not supported. |
Just one use case to show you the motivation behind it. When using version 3 we used to have the following code
When moving to version 4, someone just followed the naive substitution, which was allowed by the library interface
Which of course is now wrong, but one needs to read the library implementation to realize it. |
@@ -29,13 +28,13 @@ class Amplitude { | |||
/// // If care about init complete | |||
/// await amplitude.isBuilt; | |||
/// ``` | |||
Amplitude(this.configuration, [MethodChannel? methodChannel]) { | |||
Amplitude(Configuration configuration, [MethodChannel? methodChannel]) { |
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.
As discussed, if you could revert the changes to hiding Configuration
for now and leave in making isBuilt
final I'd be happy to approve this and get this merged!
TL;DR This PR achieves what is described in this link: https://amplitude.com/docs/sdks/analytics/flutter/flutter-sdk-4-0-migration-guide#configuration
Description
The purpose of these changes is because having both configuration and isBuilt publicly mutable is misleading, since changing those attributes will have no impact.
For example, currently it is possible to initialize Amplitude
And then write
Thinking this would lead to a on-the-fly reconfiguration of amplitude, which is not true.
Similarly but less important, I made isBuilt final since previosuly it would be possible to modify it, which makes no sense.