Rename close event to disconnect #3773
Conversation
|
As described in the EIP this change has to be a strict superset, which means we are required to support the close event as well. This is what it does right now |
|
in order to be backwards compatible we probably still need to keep around the close event listener right? |
|
@frankiebee Yeah that's why I included the close event. |
scripts/e2e.ganache.core.sh
Outdated
| @@ -11,6 +11,7 @@ set -o errexit | |||
| # Install ganache-core | |||
| git clone https://github.com/trufflesuite/ganache-core | |||
| cd ganache-core | |||
| git checkout tags/v2.13.0 | |||
There was a problem hiding this comment.
is this because we need that version for disconnect event?
There was a problem hiding this comment.
@frankiebee Not really. First of all I don't see why we should take the newest develop branch commit as they have stable releases. Second, and more importantly, I stumbled upon this because the e2e Tests with ganache failed. The reason is that they added a patch file to their package.json after install script which should change a version of a dependency. But weirdly this patch file is broken. So rolling back to the release 2.13.0 fixed the issue.
I will make a PR at ganache-core to fix this issue and after that we can use the newest version again. But I would always add an explicit version to prevent tests from failing at some point with the same commit they passed before.
Fixes #3699
We will need to make sure that the close event is still added for old providers. Either by comparing versions or by registering both, which would lead to the same warning Metamask users experience right now for newer providers.