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

fix(google-maps): unable to subscribe to events after initialization #17845

Merged
merged 1 commit into from Dec 4, 2019

Conversation

@crisbeto
Copy link
Member

crisbeto commented Nov 30, 2019

As things are set up right now in the google-map, map-marker and map-info-window components we choose whether to bind an event based on whether there are subscribers to its observable. The problem is that if the user decides to subscribe after init (e.g. by getting a hold of object with ViewChild and subscribing) it'll never emit because the native event wasn't added.

These changes fix the issue by switching the event to be observables and binding the event when something has subscribed. I've also introduced a MapEventManager class to make it easier to keep track of the events and to avoid duplication between the three components.

@crisbeto crisbeto requested a review from angular/dev-infra-components as a code owner Nov 30, 2019
@googlebot googlebot added the cla: yes label Nov 30, 2019
@jelbourn jelbourn requested a review from mbehrlich Dec 2, 2019
Copy link
Member

jelbourn left a comment

Extremely good change, just one naming nit

src/google-maps/map-event-manager.ts Outdated Show resolved Hide resolved
src/google-maps/map-event-manager.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

mbehrlich left a comment

Looks great! If it's not too much trouble, can you update the most recent component that was just added, MapPolyline as well?

As things are set up right now in the `google-map`, `map-marker` and `map-info-window` components we choose whether to bind an event based on whether there are subscribers to its observable. The problem is that if the user decides to subscribe after init (e.g. by getting a hold of object with `ViewChild` and subscribing) it'll never emit because the native event wasn't added.

These changes fix the issue by switching the event to be observables and binding the event when something has subscribed. I've also introduced a `MapEventManager` class to make it easier to keep track of the events and to avoid duplication between the three components.
@crisbeto crisbeto force-pushed the crisbeto:map-events-after-init branch from 2de37ac to 1758d40 Dec 4, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Dec 4, 2019

I've addressed the feedback @jelbourn @mbehrlich.

Copy link
Member

jelbourn left a comment

LGTM

@jelbourn jelbourn merged commit 22fecb3 into angular:master Dec 4, 2019
11 checks passed
11 checks passed
ci/angular: merge status All checks passed!
ci/circleci: api_golden_checks Your tests passed on CircleCI!
Details
ci/circleci: bazel_build Your tests passed on CircleCI!
Details
ci/circleci: build_release_packages Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: tests_browserstack Your tests passed on CircleCI!
Details
ci/circleci: tests_local_browsers Your tests passed on CircleCI!
Details
ci/circleci: tests_saucelabs Your tests passed on CircleCI!
Details
ci/circleci: view_engine_test Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.