Skip to content

Refactor Event Listeners #289

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

Merged
merged 5 commits into from
Mar 19, 2019
Merged

Refactor Event Listeners #289

merged 5 commits into from
Mar 19, 2019

Conversation

wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Mar 17, 2019

While working on iOS's event listeners I realized that the platforms have a bunch of spurious differences that don't really need to be there.

Highlights:

  • Rename ExecutorEventListener to AsyncEventListener to match iOS
  • Move ListenerRegistrationImpl and AsyncEventListener to core
  • Split ActivityScoping out of ListenerRegistrationImpl
  • Make the layering of ListenerRegistration clearer.

Note: this is running slightly in advance of forthcoming iOS changes

wilhuff added 4 commits March 16, 2019 17:51
  * Pull activity-scoping out ListenerRegistrationImpl--it's a totally
    separate concern.
  * Make the relationship between the various levels of listener clearer
    in the places where we construct them.
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review! LGTM except a few nits.

firestore.getClient(), queryListener, activity, wrappedListener);
QueryListener queryListener = firestore.getClient().listen(query, options, asyncListener);

return ActivityScope.bind(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nifty.

@mikelehen mikelehen assigned wilhuff and unassigned mikelehen Mar 18, 2019
@wilhuff
Copy link
Contributor Author

wilhuff commented Mar 19, 2019

/test smoke-tests-debug

@google-oss-bot
Copy link
Contributor

@wilhuff: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
smoke-tests-debug 50da8b3 link /test smoke-tests-debug

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@wilhuff wilhuff merged commit 4143a0a into master Mar 19, 2019
@wilhuff wilhuff deleted the wilhuff/event-listener branch March 19, 2019 19:53
@firebase firebase locked and limited conversation to collaborators Oct 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants