Skip to content

feat: add support for unregistering event handlers #283

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 4 commits into from
Jul 6, 2020

Conversation

jlengstorf
Copy link
Contributor

Refactor callback management to use Map so that we can unregister
handlers without changing the current behavior.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jlengstorf Jason Lengstorf
Refactor callback management to use `Map` so that we can unregister
handlers without changing the current behavior.

Co-authored-by: Cassidy Williams <cassidy@netlify.com>
@netlify
Copy link

netlify bot commented Jul 2, 2020

Deploy Success!

Built with commit 61bd509

https://deploy-preview-283--identity.netlify.app

Copy link
Contributor

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

why not use a Set? Should do the same right?

@jlengstorf
Copy link
Contributor Author

@mraerino no real reason — we were thinking of key/val when we walked through solutions, and I didn't actually realize that Set had delete available 😂

@cassidoo cassidoo added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jul 2, 2020

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jlengstorf Jason Lengstorf
erezrokah added 2 commits July 5, 2020 15:46

Verified

This commit was signed with the committer’s verified signature.
erezrokah Erez Rokah

Verified

This commit was signed with the committer’s verified signature.
erezrokah Erez Rokah
@erezrokah erezrokah self-requested a review July 5, 2020 12:53
@erezrokah
Copy link
Contributor

@jlengstorf, I added some tests and allowed doing .off("login") so the caller doesn't have to maintain a reference to the original callback.

Also handled a case where calling off before on would throw (I think it should be a no-op in that case, we could also throw a designated error).

If we're all good with the changes I think this is ready to merge.

@jlengstorf
Copy link
Contributor Author

thanks for adding those tests, @erezrokah!

Copy link

@philhawksworth philhawksworth left a comment

Choose a reason for hiding this comment

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

This looks good to me

@jlengstorf jlengstorf merged commit 8407a21 into netlify:master Jul 6, 2020
@jlengstorf jlengstorf deleted the feat/unregister-callbacks branch July 6, 2020 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants