Plugin State Fixes #408
Plugin State Fixes #408
Conversation
This is a clearer error message format indicating why a socket gets closed.
The tracked error object must be `unknown` and then normalized to Error on the way out of the Socket interface. We do proper type checking on the way out.
| const err = error ?? new Error('Socket closed without error') | ||
| log.warn(`onSocketClose with server ${uri}: ${err.message}`) |
There was a problem hiding this comment.
It is never valid to access error.message unless we first determine that the error is instanceof Error. Your handleError signature up above is wrong, since it should be e: unknown for the parameter type, and then follow the TypeScript squiggles until you've handled all the cases.
It might make sense to rename let error: Error | undefined to let errorMessage: string | undefined, and then handleError would have code like errorMessage = e instanceof Error ? e.message : String(e). Then onSocketClose wouldn't need to call new Error, since we are dealing with plain strings now.
Edit: Oh, derp. You end up doing something similar over the next few commits. I still think you should do the string thing, and maybe smoosh the error-handling commits together into one.
There was a problem hiding this comment.
I think keeping the internal state as an unknown time is useful for debugging what gets thrown by a breakpoint in onSocketClose. Making onSocketClose the top level handler of the thrown thing will make it easier to figure out what weird thing happens at a lower level. For example, if we see '[object Object]' errors, then we know some weird object is thrown that cannot be converted to a string. And we can debug it by going to this one place in onSocketClose to figure out what the object is, instead of trying to search for where the throw occurred.
src/common/plugin/PluginState.ts
Outdated
| @@ -222,11 +229,11 @@ export function makePluginState(settings: PluginStateSettings): PluginState { | |||
| }, | |||
|
|
|||
| async updateServers(settings: UtxoUserSettings): Promise<void> { | |||
| if (settings === userSettings) return | |||
There was a problem hiding this comment.
So we added this, and then removed it? Maybe squash the commits to avoid the detour?
There was a problem hiding this comment.
It was relevant for the previous change but not for this one. This was one of the reason why I wanted to perhaps have a second PR for these changes since this is really a bug fix on top of the other fix...
I can actually remove the previous line since it's just an early exit/optimization on the previous fix. This would make the PR more cohesive.
There was a problem hiding this comment.
Note the last commit change a little by effect of this: 1f3e346#diff-e333c8da8576d31b705ec8d0d92bea632b797f90b76f50b2c961371dd8570b2fR233
I move the constant closer to its place of use.
samholmes
force-pushed
the
sam/blockbook-disconnect
branch
from
9deae2b to
1f3e346
Compare
This will be saved and loaded to/from disk so that during initialization we have accurate `enableCustomServers` state.
By relying on `serverCache` exclusively for our most up-to-date state when calling `refreshServers` we can ensure that our initial load time and subsequent calls are using the correct state for the wallet. This fixes a race condition during initial load time: `userSettings` state was the set to `defaultSettings`. Now that we persists the last user's settings in the `serverCache` under the `'customServers'` key, we can use that instead. We can eliminate the need for a `userSettings` instance variable by using a parameter to pass the new customer servers when invoking `refreshServers`. It never made sense to rely on instance variables, or properties, to pass state around. This new approach is the correct way to manage state.
samholmes
force-pushed
the
sam/blockbook-disconnect
branch
from
1f3e346 to
98dd3a5
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none