site-kit-wp: Remove legacy code from error stores.

Feature Description

In order to help tidy up the codebase and make the error store code easier to follow, we should remove the legacy usage of the single error object with no associated baseName and args.

Additionally, we should remove the code that adds the selectorData property in getErrorForSelector(), as this has been identified as prone to error; it expects the error to be spreadable, but in fact, it’s possible for it to be an Error instance which cannot be spread into a new object without losing information.

As we now have getSelectorDataForError(), getErrorForSelector() should be made analogous to getErrorForAction() as a simple wrapper to getError() that provides a bit of additional semantic value.

Any usage of selectorData retrieved via getErrorForSelector() should be replaced with getSelectorDataForError().


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Remove usage of the single error object with no associated baseName and args:
    • The single error object should be removed from Redux state.
    • Any usage of receiveError() without baseName and args should be removed (there is only one case of this at the time of writing, but it’s in createFetchStore(), so it’s being called in a lot of places).
    • The baseName parameter should be mandatory for receiveError(), getError(), and clearError(), while args can default to an empty array.
    • All associated legacy code should be removed.
    • Ensure the above changes to createFetchStore() do not have any impact on current error handling.
  • Update getErrorForSelector() to remove the addition of selectorData, and return the error as-is.
  • Replace any usage of selectorData retrieved via getErrorForSelector() with getSelectorDataForError().

Implementation Brief

  • Inside /assets/js/googlesitekit/data/create-error-store.js locate receiveError, clearError, getError functions and make the following changes:
    • Make the baseName parameter to be mandatory for receiveError, getError, and clearError by using invariant.
      • Ensure all calls to these functions now receive baseName
    • Make the args parameter to be [] by default for receiveError and clearError
    • Change getErrorForSelector to return only error directly. It should not return an object with selectorData, and not to spread the error variable.
  • Update existing usage of const { selectorData } = getErrorForSelector( …args ) to const selectorData = getSelectorDataForError( …args ).

Test Coverage

  • All existing tests should pass; no new tests are needed.

QA Brief

  • There’s no user facing changes .For a general sanity check, make sure all the error states are working and retryable errors are showing the Retry Button.
  • QA:Eng - a secondary round of Code inspection and general test would be useful, specially making sure nothing was missed.

Changelog entry

  • Remove legacy error handling code from plugin.

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Comments: 16

Most upvoted comments

@techanvil Works for me! 👍🏻 I’ll move this back to IB now 🙂

Have created and added AC, please review.

cc @aaemnnosttv