Skip to content

fix(zone.js): Closes #31643, should remove on symbol property after removeAllListeners#31644

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:on-event-remove
Closed

fix(zone.js): Closes #31643, should remove on symbol property after removeAllListeners#31644
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:on-event-remove

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion commented Jul 19, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #31643

document.onclick = () => console.log('click');

// After calling this, the listener above is removed, and clicking on
// the document does nothing.
document.removeAllListeners();

// The 'onclick' property still contains the old event listener.
console.log(document.onclick);
console.log(document[Zone.__symbol__('ON_PROPERTYclick')]);

after removeAllListeners, the document.onclick should be null.

What is the new behavior?

after removeAllListeners, the document.onclick will be null.

Does this PR introduce a breaking change?

  • Yes
  • No

@JiaLiPassion JiaLiPassion requested a review from a team July 19, 2019 05:52
@mhevery mhevery added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release labels Jul 22, 2019
@ngbot
Copy link
Copy Markdown

ngbot Bot commented Jul 22, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci-codefresh-bazel" is failing
    pending status "ci/circleci: test_docs_examples_ivy" is pending
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Jul 22, 2019

MERGE ASSISTANCE: unrelated failures

mhevery pushed a commit that referenced this pull request Jul 24, 2019
@JiaLiPassion @mhevery
fix(zone.js): should remove on symbol property after removeAllListene…
c73b75f 
…rs (#31644)

Close #31643

PR Close #31644
@mhevery mhevery closed this in a182714 Jul 24, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@JiaLiPassion @sabeersulaiman
fix(zone.js): should remove on symbol property after removeAllListene…
35f4c08 
…rs (angular#31644)

Close angular#31643

PR Close angular#31644
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Sep 15, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mhevery mhevery mhevery approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JiaLiPassion @mhevery @googlebot