-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
fix(WebWorker): Add zone support to MessageBus #4053
Conversation
fc4acc5
to
51fa46e
Compare
@@ -38,18 +40,10 @@ class MultiClientServerMessageBus implements MessageBus { | |||
}); | |||
} | |||
|
|||
void resultReceived() { | |||
void resultReceived(e) { |
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.
It is customary to use _
for required but unused parameters. However, in this case, why do you need the parameter?
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.
It needs to have a parameter so that I can pass the function directly to onResult.listen above. Without it it won't match the argument signature. I'll change it to an underscore.
51fa46e
to
98650f6
Compare
|
||
/** | ||
* Assigns this bus to the given zone. | ||
* Any channels which are set up to run in the zone will use this zone. |
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.
It is not clear what "use this zone" means. How about "Any callbacks attached to channels created from this
[MessageBus] will be executed within the given [zone]"?
98650f6
to
cda269b
Compare
} | ||
|
||
abstract class GenericMessageBusSource implements MessageBusSource { | ||
Stream stream; |
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.
Do we need this field to be public?
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.
Can it at least be read-only?
ee89542
to
43ddd3b
Compare
/** | ||
* Assigns this source to the given zone. | ||
* Any channels which are set up to run in the zone will use this zone. | ||
*/ |
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.
Let's use similar wording as one in MessageBus.attachToZone
doc comment.
43ddd3b
to
2f27472
Compare
2f27472
to
0eafb0a
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.