Better Error handler#109
Conversation
drewstone
left a comment
drewstone
left a comment
There was a problem hiding this comment.
Thanks for adding comments to the code, it's really helpful!
The design of this seems great and well-intentioned. I am not sure about adding hardcoded delays into the code as that does seem like something we should avoid. How and why are we not able to wait until certain processes are loaded? A 3 second delay seems arbitrary and could provide issues in the future.
It seems we're just using this error handler in api init correct? Just want to make sure I'm forming a good mental model. The plan is to use this everywhere we have errors right?
| id: 1, | ||
| group: 'edgware', | ||
| tag: 'dev', | ||
| evmId: undefined, |
There was a problem hiding this comment.
If we use enums then we should make sure:
- Edgeware mainnet is 2021
- Edgeware beresheet is 2022
There was a problem hiding this comment.
Will handle this on a refactor PR after the merge for #111
| const [apiPromise, injectedExtension] = await PolkadotProvider.getParams(appName, endpoints); | ||
| async awaitMetaDataCheck() { | ||
| /// delay some time till the UI is instantiated and then check if the dApp needs to update extension meta data | ||
| await new Promise((r) => setTimeout(r, 3000)); |
There was a problem hiding this comment.
This isn't great practice, can we remove this? What errors occur when you do?
There was a problem hiding this comment.
This is checking for the metadata update(which isn't really urgent at init) doing timeout will make this task of a lower priority although timeout of 0 is enough,so by the time the timeout is done the UI would have been initialized instead of UI freezes, Polkadot ApiPromise seems heavy already it freezes the UI for 2 seconds just don't wont to add to this as the update metadata will trigger a modal, But I hear you will test it more and improve it
| let interActiveFeedback: InterActiveFeedback; | ||
| const body = InterActiveFeedback.feedbackEntries([ | ||
| { | ||
| header: 'Failed to establish WS connection', |
There was a problem hiding this comment.
we should probably abstract all our error messages into a common folder/location as well and use Enums.
There was a problem hiding this comment.
Added implementation for the Error here https://github.com/webb-tools/webb-dapp/blob/c936d7541f44e5216b6f833790710993deb39843/packages/react-components/src/InteractiveFeedbackView/InteractiveErrorView.tsx#,
and as you suggested we can handle the content for error messages
| webbWeb3Provider.on('providerUpdate', async ([chainId]) => { | ||
| const nextChain = Object.values(chains).find((chain) => chain.evmId === chainId); | ||
| if (!nextChain) { | ||
| try { |
There was a problem hiding this comment.
Should we be trying anything if we do not support the evm chain id? can we just put the entire catch block with case: UnsupportedChain here?
There was a problem hiding this comment.
this call WebbWeb3Provider.storageName(chainId) will throw UnsupportedChain Error catching this and show an interactiveFeedback
…-handler # Conflicts: # packages/react-environment/src/WebbProvider.tsx # packages/react-environment/src/api-providers/web3/webb-web3-provider.ts
3 participants
No description provided.