Remove legacy code from error stores.

[techanvil]

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.

[techanvil]

Have created and added AC, please review.

cc @aaemnnosttv

[aaemnnosttv]

Thanks @techanvil – I think the simpler change would be to keep getErrorForSelector but make it closer to how it was initially which only returned the error. As part of this change we would need to leverage the new selectors in some places as you mentioned in order to provide the same information where needed of course. In the future we could perhaps do more to make sure an error is only returned for a selector by the same name, but I'm not sure we need to do that much here. I believe we had discussed that in the past at some point and we identified that there shouldn't be a case where such a naming collision would happen, but also I think it would probably require us to store errors for actions and selectors separately which again doesn't seem to be necessary at this point.

[techanvil]

Hey @aaemnnosttv, I must confess I don't see much benefit in keeping getErrorForSelector but only returning the error from it. To me the selector would then be a bit confusingly named as it would essentially just be a wrapper for getError.

Seeing as we now have the getSelectorDataForError selector which does check whether the named selector exists, it strikes me that this, in combination with getError would be enough to provide the behaviour we need.

Incidentally looking through the existing usage of getErrorForSelector it's evident that in some cases the selectorData is not being used, which raises the question of why getErrorForSelector is used there rather than getError. It strikes me that removing getErrorForSelector, instead using getError and only using getSelectorDataForError in cases where we do need the selector data could be a bit more clear in terms of the intent of the code.

Does the above change the calculus for you at all or do you still think we'd be better off keeping getErrorForSelector? It could be I've missed a good reason to keep it.

[aaemnnosttv]

@techanvil I'm not strongly against removing it, but if we decide that it isn't worth keeping in place of using getError directly then it suggests we should also consider doing the same for getErrorForAction. I think part of the reason for the separation in the past (as a layer on top) was that getError supported calling without any arguments, to get a "main" error. The action and selector variants enforce the requirement for the baseName argument. I don't think we use the "main" state.error anymore so this could be addressed together which would resolve this TODO (there is at least one more of these around – in createFetchStore, maybe others?)

site-kit-wp/assets/js/googlesitekit/data/create-error-store.js

Lines 275 to 278 in 2a24f57

	// @TODO: remove it once all instances of the legacy usage have been removed. Also make baseName required then.
	if ( ! baseName && ! args ) {
	return error;
	}

The separation of separate selectors for actions and selectors didn't turn out to be really necessary I suppose but this may have been done before we had more well-established conventions for their respective naming.

How about we revise the definition to address these other related bits?

[techanvil]

@aaemnnosttv thanks for your reply. Actually, I have to say, your explanation has made me reconsider things a bit. I think I was being a bit blinkered and focusing overly on the selectorData aspect of things. Having getErrorForSelector and getErrorForAction is quite useful for providing a bit of extra semantic/contextual information, plus searchability and potential for customisation. I think where it can start to get a bit confusing especially when in the depths of the code is the fact there's no separation of these errors at the point of where they are stored. It would be nice if there was a converse setErrorForSelector / setErrorForAction to provide similar clarity at set-time and open up the possibility of a segregated storage by type.

Maybe we should update this issue to be about removing selectorData from getErrorForSelector, and also removing the legacy "main" error. Sorry for the flip flopping, but how does that sound to you?

[aaemnnosttv]

Having getErrorForSelector and getErrorForAction is quite useful for providing a bit of extra semantic/contextual information, plus searchability and potential for customisation.

Yes I think that was part of the original intention as well. If we feel it's still useful, then let's keep it :)

Maybe we should update this issue to be about removing selectorData from getErrorForSelector, and also removing the legacy "main" error.

Sounds good to me 👍

As for changing the way things are stored perhaps we should open a new issue for that to think through it thoroughly. Thanks!

[mxbclang]

@techanvil Checking in here, thanks!

[techanvil]

Thanks @bethanylang, I've not forgotten about this, just had a number of things on my plate and have been trying to balance those with the ongoing sprint requirements as usual. I should be getting round to this one soon.

[techanvil]

Maybe we should update this issue to be about removing selectorData from getErrorForSelector, and also removing the legacy "main" error.

Sounds good to me +1

As for changing the way things are stored perhaps we should open a new issue for that to think through it thoroughly. Thanks!

Hey @aaemnnosttv, I've updated this issue accordingly, and also created #6568 to consider the additional changes, which I've left in Triage.

[tofumatt]

ACs here are good; the last few points are more like an IB so I noted they're likely implementation details, but moved to IB 👍🏻

[techanvil]

Hey @tofumatt, I appreciate those points are quite specific in terms of code but they are part of what we are explicitly trying to achieve in this issue (see the second and last paragraphs of the Feature Description).

I've updated the AC to hopefully make it a bit clearer, please take another look.

[tofumatt]

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

[tofumatt]

I've updated the IB a bit and added an estimate, but this can be moved to the backlog now.

IB ✅

[kuasha420]

Unassigned myself as I'm going on holidays, I've pushed all my progress here.

[wpdarren]

QA Update: ✅

Verified:

• We have over 65 error states according to this spreadsheet, so I am unable to test them all. I did take a wide section of error states and retry buttons and there were no user facing changes.

Screenshots

Added QA:Eng label

[zutigrm]

QA Eng Verified ✅

• Went through general errors Darren also checked
• The spreadsheet of errors @wpdarren linked, seems to cover more general types of code errors, some seems outdated and mostly not related to the change here, but I went through general error notice, by intentionally breaking a piece of the code
• Tested additional retrievable errors - around custom dimensions and frontend Site Kit widget in admin bar when user is offline
• Verified the error surfaced when Analytics property/web data stream that is saved is deleted in Analytics admin
• Verified that error is showing in WordPress dashboard widgets

Nothing stood out