Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(jqLite): forgive unregistration of a non-registered handler #3465

Conversation

pkozlowski-opensource
Copy link
Member

I've bumped into a subtle bug when unregisering handlers with jqLite. If you try to unregister a non-existing handler by specifing both its type and a function jqLite's off will fail if there was already a different handler was registered. See the test for more details.

With jQuery this situation is handled correctly, that it, unregistration has no effect.

I was hesitating a bit between fixing it in jqLite or fixing directly the indexOf / arrayRemove (in Angular.js). In any case the test illustrates my real problem, can amend the fix if needed.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@pkozlowski-opensource
Copy link
Member Author

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name) - signed, signed, @mary-poppins got it wrong ;-)
  • PR doesn't introduce new api
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • [- ] PR contains e2e tests (if suitable)
  • [- ] PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@petebacondarwin
Copy link
Contributor

Dear @pkozlowski-opensource - please ensure that you have signed the CLA :-P

@IgorMinar
Copy link
Contributor

LGTM

@pkozlowski-opensource
Copy link
Member Author

@IgorMinar OK, great, so I'm going to push the fix to master plus a different one for stable (as we've changed unbind to off and as such we can't merge the same code).

@pkozlowski-opensource
Copy link
Member Author

Landed as ab59cc6 and ac5b905.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants