feat: catch and log electron-store.set errors

[SgtPooki]

Technically, this is a partial fix for #2336 because we're not sure which value
or key is being used when store.set was triggering the error in that issue.

• feat: add store.safeSet function and fix tests
• feat: use store.safeSet

fixes #2336

[whizzzkid]

questions and concerns around safeStoreSet

[whizzzkid]

Do we absolutely need to pass a callback? I would rather return a success boolean and let the caller decide what needs to happen next.

But if we want to use it, return await is required here (because try-catch) also, can we check typeof onSuccessFn === 'function' or typeof onSuccessFn instanceof Promise before calling it?

[SgtPooki]

I do think we should have some sort of success identifier, regardless of what it is. Part of the problem with the existing code is that we assume the values we're setting are actually being set even though they may not be.

In general, I am not a fan of putting the burden on consumers for APIs. If we were to return a true/false, then the responsibility of control flow is on the consumer to check whether they set things successfully or not, and then call their own function. If we were going to do that, it would be better to just wrap all store.set() calls in a try/catch where it's used, instead of using safeStoreSet, but I feel like that's a lot for using a simple keyValue store.

return await is required here (because try-catch)

try-catch doesn't necessitate the use of return await. A return await keeps the awaited promise inside the call-stack, instead of throwing separately.

By not awaiting the onSuccessFn, we require consumers to handle errors in their own function. We don't want to have safe-store-set be responsible for catching and logging errors for functions completely unrelated to the store, which is generally what the onSuccessFn are used for. e.g. set new apiUrl, then trigger ipfs-daemon restart.

[SgtPooki]

we could await the onSuccessFunction and then throw a separate error there while identifying the key used

[whizzzkid]

I am trying to understand if it's only the try-catch that's providing safety.

Can we not add safety to the existing implementation by using a proxy? Maybe something like:

const store = new Store({
  defaults,
  migrations
})

const patchedImplementations = {
  set: (target) => async (key, value, onSuccessFn) => {
    try {
      target.set(key, value)
      return await onSuccessFn?.()
    } catch (err) {
      logger.error(`Could not set store key '${key}' to '${value}'`, /** @type {Error} */(err))
    }
  }
}

const patchedStore = new Proxy(store, {
  get (target, prop, receiver) {
    if (prop in patchedImplementations) {
      return patchedImplementation[prop](target);
    }
    return Reflect.get(...arguments);
  }
});

[SgtPooki]

I explicitly didn't use a proxy because we use the store so often, but I don't imagine it would hurt too much, and it would be significantly less code.

[SgtPooki]

I looked into this and using the proxy is odd because the store is supposed to be electron-store and we'd actually be modifying the API to accept a third parameter, instead of explicitly calling a separate method.

[whizzzkid]

@SgtPooki missed your notification, github needs to fix this.

do we need to explicitly pass a callback? maybe the set implementation needs to be just:

const patchedImplementations = {
  set: (target) => (key, value) => {
    try {
      target.set(key, value)
    } catch (err) {
      logger.error(`Could not set store key '${key}' to '${value}'`, /** @type {Error} */(err))
    }
  }
}

// and then wherever you call it:

store.set(key, value) //already safe now this error can be caught?

[whizzzkid]

As a follow up when TS might be available, this can still be a proxy, with modified ambient type:

declare interface SafeStore extends Electron.store {
    set: (key, value, optionalFn) => void
}

then use that work with store eventually.

[SgtPooki]

self review

[SgtPooki]

fix error logging

[SgtPooki]

I looked into this and using the proxy is odd because the store is supposed to be electron-store and we'd actually be modifying the API to accept a third parameter, instead of explicitly calling a separate method.

[SgtPooki]

using the real electron-store, but removing any migrations to prevent CI failures.

[SgtPooki]

self review

[whizzzkid]

[whizzzkid]

TYSM ❤️