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

Add removeAllListeners method #1288

Merged
merged 17 commits into from
May 1, 2024
Merged

Add removeAllListeners method #1288

merged 17 commits into from
May 1, 2024

Conversation

joaniefromtheblock
Copy link
Contributor

@joaniefromtheblock joaniefromtheblock commented Apr 24, 2024

Description

Adds removeAllListeners method and example to docs

Issue(s) fixed

Fixes #563

Preview

https://docs.metamask.io/add-listen/wallet/reference/provider-api/#remove-listeners

Checklist

Complete this checklist before merging your PR:

  • If this PR contains a major change to the documentation content, I have added an entry to the top of the "What's new?" page.
  • The proposed changes have been reviewed and approved by a member of the documentation team.

@joaniefromtheblock joaniefromtheblock requested review from a team as code owners April 24, 2024 20:47
Copy link

Preview published: add-listen

Copy link

Preview published: add-listen

Copy link

Preview published: add-listen

Copy link

Preview published: add-listen

@httpJunkie
Copy link
Contributor

httpJunkie commented Apr 25, 2024

Screenshot 2024-04-25 at 5 20 34 PM

Considering the current version of the page:

When we are talking about the code that has the removeAllListeners() below the code box. We say that this happens on component unMount which is putting the example in context of a React application. The code in the example is ok, because the code can make sense in any JS code environment.

My concern is that the explanation mentions something that happens in React, but if this were plain javascript code, we might just want to say:

Text above the code (Warning)

Use removeAllListeners with caution. This method clears all event listeners associated with the emitter, not only the listeners set up by other application code. Using this method can unexpectedly clear important event handlers, interfere with scripts, and make debugging more complex. You can use the method removeListeners to safely remove specific listeners.

Text below the code:

In the provided code example, removeAllListeners is called to remove all event listeners attached to the window.ethereum object. This cleanup callback function ensures that when the code block is no longer needed, there are no lingering event listeners.

@httpJunkie
Copy link
Contributor

The code example could be reduced to:

window.ethereum.on('_initialized', updateWalletAndAccounts);
window.ethereum.on('connect', updateWalletAndAccounts);
window.ethereum.on('accountsChanged', updateWallet);
window.ethereum.on('chainChanged', updateWalletAndAccounts);
window.ethereum.on('disconnect', disconnectWallet);

return () => {
  window.ethereum.removeAllListener()

This example is more efficient code that instead of setting multiple .on()'s we are just calling some function that does everything needed for that specific event. It also is familiar to those working with MetaMask and doesn't introduce calls to SDK methods that don't make sense in the context of this reference.

This will reduce the complexity of the example in a way that doesn't compromise the illustrated point.

Copy link

Preview published: add-listen

wallet/reference/provider-api.md Outdated Show resolved Hide resolved
wallet/reference/provider-api.md Outdated Show resolved Hide resolved
wallet/reference/provider-api.md Outdated Show resolved Hide resolved
wallet/reference/provider-api.md Outdated Show resolved Hide resolved
wallet/reference/provider-api.md Outdated Show resolved Hide resolved
wallet/reference/provider-api.md Outdated Show resolved Hide resolved
wallet/reference/provider-api.md Outdated Show resolved Hide resolved
Co-authored-by: Alexandra Tran Carrillo <12214231+alexandratran@users.noreply.github.com>
Copy link

Preview published: add-listen

Copy link

Preview published: add-listen

Copy link

Preview published: add-listen

Copy link

Preview published: add-listen

Copy link
Contributor

@alexandratran alexandratran left a comment

Choose a reason for hiding this comment

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

LGTM once suggestions are addressed

docs/whats-new.md Outdated Show resolved Hide resolved
wallet/reference/provider-api.md Outdated Show resolved Hide resolved
joaniefromtheblock and others added 2 commits April 30, 2024 13:09
Co-authored-by: Alexandra Tran Carrillo <12214231+alexandratran@users.noreply.github.com>
Copy link

Preview published: add-listen

1 similar comment
Copy link

Preview published: add-listen

Copy link

github-actions bot commented May 1, 2024

Preview published: add-listen

Copy link

github-actions bot commented May 1, 2024

Preview published: add-listen

@joaniefromtheblock joaniefromtheblock merged commit 8fabc70 into main May 1, 2024
8 checks passed
@joaniefromtheblock joaniefromtheblock deleted the add-listen branch May 1, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add example to docs for RemoveAllListeners
3 participants