-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Stop reloading dapps on network change allowing dapps to decide if it should refresh or not #6330
Stop reloading dapps on network change allowing dapps to decide if it should refresh or not #6330
Conversation
Wow, my first impression: Wonderfully elegant solution to a problem that felt like it stuck us between a rock and a hard place. And a PR no less! I'm going to share this with the team a little bit more, but this looks simple enough to intelligently merge it pretty eagerly. Thanks a ton for the contribution! |
One non-blocking thought is we could move the flag into the |
Awesome news! Super happy you like it. I was going to do a PR in the metamask-inpage-provider but I didn’t want to have to do 2 PRs on 2 different repos, I wanted to address the solution and outline it like I have in 1 PR first. Like you said moving it into the inpage provider repo wouldn’t be hard and can come next. As you guys appreciate dapps now are becoming a lot more smarter and this will move us in the correct direction to allow more complex state driven dapps to work without a reload. Thanks for being so welcoming and supportive with the solution I presented. In terms of merging do keep me updated with the discussion with the team etc, the sooner we could get this in the better! Cheers Dan 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so we're willing to merge this as an "experimental feature" for now. Going forwards, things we will want to do before we consider it finalized (you can help if you'd like):
- Get it in our
metamask-docs
repo (listing its experimental status) - Open an EIP and optionally an ethereum-magicians post to bring the change to the attention of other client developers.
Once a version of this is merged as an EIP, we will consider it stable and update our docs to reflect that. For now, let it be considered an experiment that will be live soon!
Bonus: Long term we are hoping to allow ways of requesting additional networks without user involvement, and so long term it should be increasingly common for users to not ever be directed to select a network, new apps should all have what they need via constant API, reducing the danger of not setting this flag in the future.
Thanks Dan that’s awesome news! Does the experimental features actually go live? so we can expect this in the next release? I can do those things above, il get that sorted and PRs up for you tomorrow to finalise everything, anything else I can do to help give me a shout! Thanks for your quick response and fast merge. great stuff guys!! |
Oh and also you may want to close/explain on this bug what has happened, as this is where the whole debate came from. 👍 |
Hey @danfinlay docs have been updated here - MetaMask/metamask-docs#9 👍 |
* Fixes the 'Percentages row tracks and gutters' decpracation warning. (#6244) * Fix missing this reference in addtoken button onclick method (#6245) * Only call onRecipient and onSender methods if defined in sender-to-recipient (#6247) * Prevent advanced gas input arrows from setting value to < 0 (#6248) * Add privacy policy link to modal metrics opt-in (#6250) * Handle undefined gas limits and prices in transaction-breakdown.component (#6246) * Uppercase and context fixes on Spanish translation. * development - enhancement for sourcemap validator tool (#6277) * Nonmultiple notifications for batch txs * No longer check network when validating checksum addresses * Version 6.2.1 (#6251) * Remove line rather than comment out * Fixes the use of the browser back button on the reveal seed screen * GABA: Integrate AddressBookController (#5847) * gaba: integrate AddressBookController * pin gaba version and update lockfile * ci: Use cache version from circle environment var (#6286) * Allow npm to update the package-lock.json file * npm i -D [email protected] * mascara - remove from project (#6283) * circleci - disable npm dep cache (#6288) * Centre all notification popups * ui - add missing PropTypes (#6287) * Improve Korean translations (#6268) * ci: Use npm ci for fast(er) installs * build - babel - move config to babelrc (#6284) * Patch/bump version (#6294) * Update Node minor version * ci: Skip updating npm@6 as it is default * Allow seed phrases with a trailing newline * Centre the notification in the current window (#6307) * Fixes popups not showing when screen size is odd (#6312) * Fix typos in English messages (#6317) * Add rollback script, move auto-changelog script (#6252) * Bump gaba version to avoid broken eth-contract-metadata * Fixing spelling of Ethereum in MetaMetrics copy (#6329) * Stop reloading dapps on network change allowing dapps to decide if it should refresh or not (#6330) * feat: `inpageProvider.autoRefreshOnNetworkChange` to allow dapps to control if it refreshes or not * feat: check the `autoRefreshOnNetworkChange` before a refresh * fix linting error * fix: use `window.ethereum` now `web3.ethereum` * Enable mobile sync (#6332) * enable mobile sync * remove mobile sync as a preference * Fix typo * Folder restructure (#6304) * Remove ui/app/keychains/ * Remove ui/app/img/ (unused images) * Move conversion-util to helpers/utils/ * Move token-util to helpers/utils/ * Move /helpers/*.js inside /helpers/utils/ * Move util tests inside /helpers/utils/ * Renameand move confirm-transaction/util.js to helpers/utils/ * Move higher-order-components to helpers/higher-order-components/ * Move infura-conversion.json to helpers/constants/ * Move all utility functions to helpers/utils/ * Move pages directory to top-level * Move all constants to helpers/constants/ * Move metametrics inside helpers/ * Move app and root inside pages/ * Move routes inside helpers/ * Re-organize ducks/ * Move reducers to ducks/ * Move selectors inside selectors/ * Move test out of test folder * Move action, reducer, store inside store/ * Move ui components inside ui/ * Move UI components inside ui/ * Move connected components inside components/app/ * Move i18n-helper inside helpers/ * Fix unit tests * Fix unit test * Move pages components * Rename routes component * Move reducers to ducks/index * Fix bad path in unit test * Hide gas price chart and prevent api call when not on ethereum networks. (#6300) Add missing translations in gas customization modal * Fix gas fee in the submitted step of the transaction details activity log. (#6301) * Fix oversized loading overlay on gas customization modal. (#6326) * Replaces the coinbase link in the deposit modal with one for wyre (#6302) * New settings page rebased (#6333) * New setting tab * Add InfoTab * Add Advanced tab * Add Security Tab * Finish mobile view * Make new setting page responsive * Fix linter * Fix y scrolling * Update link in network dropdown * Fix e2e tests * Remove duplicate translation key * Resolve merge conflict * Only change settings header in popup view. * Place mobile-sync button in advanced-tab of settings * Close transaction on close of notification window (#6340) * Cancel error rebased (#6341) * Check balance before showing cancel * Fix linter * Use existing helper methods for calculating increased cancel price * Add tooltip for disabled button * Lint fix for cancelError branch. * Disabling of cancel button should account for value of tx. * E2E - Dont close window notifications (#6349) * Dont close window notifications * Remove commented out lines in beta spec * Don't include tx value in calculation of balance sufficiency for cancel button disabling. (#6346) * enable privacy mode for first time users (#6347) * Version 6.3.0 (#6350)
Hi I am a blockchain developer who works for FunFair. We really appreciate the epic work you're doing on your extension thanks a bunch and keep it up, now the juicy part! 😄
https://medium.com/metamask/breaking-change-no-longer-reloading-pages-on-network-change-4a3e1fd2f5e7
We read this article around 9 months ago about no longer reloading the page on network change. When we first heard about it we all jumped for joy as this was a HUGE UX problem for us. Losing all the current state on refresh causes millions of issues. When you released and then straight away rolled back we were disappointed but knew why. So many dapps now depend on this behaviour so just bringing a breaking change like that just didn't make sense. 9 months later we are here now, still with the huge pain of it reloading.
Thinking closely about this I think it was a mistake deciding on a breaking change, my view is dapps should have control of this feature, dapps should be able to decide if it should reload or not leading to a best of both worlds solution. This extends on to what i have changed in this PR.
You introduced something called
ethereumProvider
which you create your web3 instance with. This holds methods likeenable()
which the dapps have to call to see if the user has given permission or not. If not then it will pop up the MetaMask screen for approval. So dapps are openly using the methods onwindow.ethereumProvider
now.What I propose is a simple easy solution which solves this. Yes it may not be the perfect solution but it allows best of both worlds until you're ready to focus on completely killing the auto-refresh.
If we added another method on the
ethereumProvider
callledautoRefreshOnNetworkChange
which is default set to true i.e. it will always reload like it does now meaning old dapps don't need to change a thing. Now if dapps want to disable this all they have to do is:on startup of the app and then it will just return in the
auto-reload.js
file if that is not true. This even gives the flexibility for some parts of the dapps (say legacy) to reload and some old dapps which can't support this straight away can change it feature by feature and roll it out.I think we know now that this isn't your main focus, you have the mobile MetaMask and loads of awesome things you're doing. But this issue has a lot of attention and is the single biggest annoyance of the extension to date as you can see by the 23 comments on this issue:
#3599
Dapps today are pushing the barriers of what they can do and to allow that to happen this needs to change ASAP. Something like this could solve all this until you're ready to kill it completely.
I am looking forward to hearing your thoughts.
Thanks for spending the time reading
Josh