site: Check for uninitialized market in certain note handlers #2286
Conversation
ukane-philemon
force-pushed
the
fix-js-errors
branch
2 times, most recently
from
fec775a to
d0c56ae
Compare
There was a problem hiding this comment.
It's not exactly clear to me under what conditions we might have no this.market, but be receiving spot and order updates. You mention connection issues. Can you explain what sequence would land us on the markets view, with spot and order updates coming in, but with no connected exchanges? Is it after a login with no available connections?
Regardless, most of these changes are unnecessary, because they require this.market to be populated in order for the requisite page elements to be shown that would lead to the error. Please audit a little deeper to see which are useful.
| @@ -690,7 +691,7 @@ export default class MarketsPage extends BasePage { | |||
|
|
|||
| if (!this.assetsAreSupported().isSupported) return // assets not supported | |||
|
|
|||
| if (this.market.dex.tier < 1) return // acct suspended or not registered | |||
| if (!this.market?.dex || this.market.dex.tier < 1) return // acct suspended or not registered | |||
There was a problem hiding this comment.
This also seems like an impossible condition to hit based on how resolveOrderFormVisibility is used.
There was a problem hiding this comment.
Not really, createWallet also calls this method and it could be that there is no dex connection by the time we use resolveOrderFormVisibility.
Will do. I agree some use of optional chaining in some methods in this might be redundant since callers might have already checked for such cases. |
While on the markets page, no connection and the page has been refreshed. Turning the internet connection on could result in some notes being sent to the markets page before it is reloaded(i.e after connection to the server has been successful). It occurs randomly since the markets page might be refreshed before the notes start hitting the FE. I will limit this safety check to only notes handlers. |
ukane-philemon
force-pushed
the
fix-js-errors
branch
from
d0c56ae to
5fb99e5
Compare
- This fixes an issue where the `markets` page receives certain notes before `this.market` is initialized. These notes are sent when a user reconnects to the internet and the market page has not been refreshed. Signed-off-by: Philemon Ukane <[email protected]>
ukane-philemon
force-pushed
the
fix-js-errors
branch
from
5fb99e5 to
5e5b87a
Compare
ukane-philemon
changed the title
|
In my previous efforts, I exercised excessive caution by proposing the adoption of JavaScript's optional chaining feature. However, following a recent discourse with @buck54321, and after considering our utilization of specific note handlers, I have come to the realization that such precaution is unnecessary, thereby rendering the implementation of js optional chaining superfluous. |
Signed-off-by: Philemon Ukane <[email protected]>
This diff uses the JS optional chaining feature to prevent errors caused by attempting to access a null field. If a user losses internet connection and regains it, some js errors will be logged when notes try to access the
dexormarketfield onMarketPagebefore those fields are initialized. But the changes here are not limited to only note handlers since it is cleaner and less code to use optional chaining to prevent runtime errors.