-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add support for flexible subscriptions #496
Conversation
ef483ff
to
2ef7e13
Compare
Pull Request Test Coverage Report for Build 2310272717
💛 - Coveralls |
9047c2f
to
7bc1673
Compare
f0ed13d
to
483a3b0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
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.
I will need more details on these changes. Why do we need to reference EventLoopDispatcher type from core. Lets have a discussion about this.
081545e
to
ffc11c0
Compare
8036945
to
dc736c3
Compare
lib/src/configuration.dart
Outdated
String? path, | ||
}) = InMemoryConfiguration; | ||
|
||
factory Configuration.flexibleSync( |
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.
why do we call it flexibleSync
. Since we are supporting only that type of sync isn't it better to name it Configuration.sync
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.
All our documentation and marketing talk about flexible sync. I think it'll be confusing if the dart SDK is the only one which doesn't specify the sync type. When/if we deprecate PBS, we can deprecate the "flexible" terminology across all SDKs.
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.
I tend to agree with @nirinchev. I'll keep as is for this PR, we can always change later.
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.
I don't agree that docs and marketing should play here how we name this exact method. Yes we have "flexible sync" type but why put it in the config method name. We don't have any other sync
. The docs could go into great detail how sync in Dart works but this should not be imposed on the method name.
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.
and we are keeping the FlexibleSyncConfiguraiton
class name
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.
Who can call this decision?
cad6f3c
to
db0ce19
Compare
Use Configuration.local factory instead.
aacf86b
to
48c8ab7
Compare
Example: