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
errorobject with no associatedbaseNameand args:- The single
errorobject should be removed from Redux state. - Any usage of
receiveError()withoutbaseNameandargsshould be removed (there is only one case of this at the time of writing, but it’s increateFetchStore(), so it’s being called in a lot of places). - The
baseNameparameter should be mandatory forreceiveError(),getError(), andclearError(), 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.
- The single
- Update
getErrorForSelector()to remove the addition ofselectorData, and return the error as-is. - Replace any usage of
selectorDataretrieved viagetErrorForSelector()withgetSelectorDataForError().
Implementation Brief
- Inside
/assets/js/googlesitekit/data/create-error-store.jslocatereceiveError,clearError,getErrorfunctions and make the following changes:- Make the
baseNameparameter to be mandatory forreceiveError,getError, andclearErrorby usinginvariant.- Ensure all calls to these functions now receive
baseName
- Ensure all calls to these functions now receive
- Make the
argsparameter to be[]by default forreceiveErrorandclearError - Change
getErrorForSelectorto return onlyerrordirectly. It should not return an object withselectorData, and not to spread the error variable.
- Make the
- Update existing usage of
const { selectorData } = getErrorForSelector( …args )toconst 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
RetryButton. - 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
@techanvil Works for me! 👍🏻 I’ll move this back to IB now 🙂
Have created and added AC, please review.
cc @aaemnnosttv