Add selectorData to all selector-related errors returned by getError / getErrors.

[techanvil]

Feature Description

This has been identified as a requirement in order to get https://github.com/google/site-kit-wp/issues/5494 working - for more info see the related comment thread in the issue.

In order to be able to show a retry button in the ErrorNotice component that allows a selector error to be retried, errors returned from getError and getErrors should be populated with selectorData when they relate to a selector.

Consequently, getErrorForSelector can be removed and usage of it replaced by getError.

---

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

Acceptance criteria

• Modify the getErrors and getError selectors to always include the selectorData on an error if applicable.
• More specifically, in getErrors, we can rely on the baseName that is stored in the error keys to define the selector name. For the selector args, we'll need to add the error args to the Redux store for reuse. Refer to this comment for a bit more detail on this. Note that not every error has baseName and args, in which case selectorData cannot be populated.
• In getError we can simply rely on the passed baseName and args parameters.
• In addition, we would need to change both getErrors and getError to use createRegistrySelector so that we can then check on the current store whether a selector with the baseName exists. If so, this error is indeed for a selector and we should amend the error object with the relevant data under selectorData. Otherwise/if not, this error is not for a selector, so we shouldn't add selectorData to it.
• We should also memoize the resulting augmented error objects to prevent rerendering issues.
• Remove getErrorForSelector and replace usage of it with getError.
• Add a bit of JSDoc documentation to clarify this relationship between baseName / args and selectorData.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

[techanvil]

Hey @aaemnnosttv, just assigned this to you for a review of the AC...

[eclarke1]

@aaemnnosttv just a polite reminder on this one please as it's for Sprint 82, plus it's blocking an issue with @makiost that's already complete and waiting to be merged

[aaemnnosttv]

@eclarke1 thanks for the reminder – I haven't forgotten, just been giving it some thought as I think we might want to take a different approach.

[techanvil]

Hi @aaemnnosttv, I have updated the AC. I did have another think about the idea of using a Map with the error as the key, and in fact fleshed out the potential AC for it. I do like the idea, but just find it a bit hard to fully get behind the thought of adding this non-serializable component to the error store. What if we want to snapshot the error store later? I suppose we could always rewrite it then to remove the Map...

To be honest, I am pretty on the fence here and would really have no objection to switching to the Map version if you prefer this route...

For the sake of argument here is the AC for a `Map` version.

Acceptance criteria

• We want the ability to look up selector data for a given error object. In order to facilitate this we'll use a Map instance, as Map allows keying by objects.

• In createErrorStore, create a new Map instance. Create it at the same level as initialState so it's closed over in the same way.

• For each action type, create a corresponding control that updates the Map.

  • RECEIVE_ERROR: Store the baseName and args, keyed by the error object. Store them as { name, args } to avoid potentially having to memoize a renamed version later.
  • CLEAR_ERROR / CLEAR_ERRORS: Lookup the error object(s) and delete their Map entries.

• Create a new error store selector, getSelectorDataForError which takes an error object as its argument.

  • Lookup the selector data, i.e. name and args, for the error in the Map.
  • Use createRegistrySelector and check for the existence of a selector by that name on the store.
  • If the selector does exist, return the selector data. Return the object instance retrieved from the Map to ensure the same object is returned each time and avoid potential rerendering issues.
  • Otherwise return null or undefined.

• getSelectorDataForError can be integrated via https://github.com/google/site-kit-wp/issues/5494 which will need to call it in ErrorNotice.

• Add a followup issue to replace usage of getErrorForSelector with getError and then call getSelectorDataForError where we need the selector data in ReportError. getErrorForSelector can be removed entirely.

[aaemnnosttv]

I have updated the AC. I did have another think about the idea of using a Map with the error as the key, and in fact fleshed out the potential AC for it.

Thanks @techanvil! It certainly helps to see things side-by-side when decisions get tricky/nuanced so I think this qualifies 😄

I do like the idea, but just find it a bit hard to fully get behind the thought of adding this non-serializable component to the error store. What if we want to snapshot the error store later? I suppose we could always rewrite it then to remove the Map...

I hear you. Regarding concerns about serializability: Error instances aren't serializable either, so as long as we're aiming for compatibility with these then that wouldn't be possible in either case.

It's also worth noting that @wordpress/data already uses Maps in the core "metadata" stores that are bundled into every store to track the invocation of selectors with resolvers. This is already not included in our store snapshotting because that state is – while part of the same redux state tree – isolated at a higher level than ours ("private" if you will). So resolver state is not preserved as part of the store snapshotting either.

With that said, I think the tradeoff with the Map is losing the ability to lookup an error's arguments after restoring a state snapshot which seems like something we should avoid.

How about we keep the ACs you have, and add a selector for getting the arguments with a given error? We should be able to reverse-lookup the error's key in the state and its args by the same key (as you have defined). Maybe that even makes some of the other changes unnecessary as we would still be able to look up the args by error alone. What do you think?

[techanvil]

Hi @aaemnnosttv, thanks for the comment/suggestion - I like the idea of doing a reverse lookup and have updated the AC, it's ended up as a bit of a mix between the two proposals.

I hear you. Regarding concerns about serializability: Error instances aren't serializable either, so as long as we're aiming for compatibility with these then that wouldn't be possible in either case.

Yup, this aspect did occur to me too... My thoughts being along the lines of, the Error object case is presumed to be the outlying, not-so-expected scenario, and we should be able to address it by updating the calls to receiveError within a catch block, or similar. It's definitely another angle we'd need to tackle if we do want to snapshot the store or otherwise ensure full serializability.

It's also worth noting that @wordpress/data already uses Maps in the core "metadata" stores that are bundled into every store to track the invocation of selectors with resolvers. This is already not included in our store snapshotting because that state is – while part of the same redux state tree – isolated at a higher level than ours ("private" if you will). So resolver state is not preserved as part of the store snapshotting either.

Thanks, I had not spotted that, useful to know! Interesting to see that the EquivalentKeyMap uses deep-equality rather than strict equivalence to determine a match for the key. This suggests it would lend itself slightly more easily to serializing - although of course, only if the keys are themselves properly serializable, e.g. not Error objects.

With that said, I think the tradeoff with the Map is losing the ability to lookup an error's arguments after restoring a state snapshot which seems like something we should avoid.

How about we keep the ACs you have, and add a selector for getting the arguments with a given error? We should be able to reverse-lookup the error's key in the state and its args by the same key (as you have defined). Maybe that even makes some of the other changes unnecessary as we would still be able to look up the args by error alone. What do you think?

As mentioned at the top - sounds good to me, and I have updated the AC accordingly (although ending up with a bit of a mix).

If you are happy with it, we could probably add an IB which just says "implement as per the AC" as the AC is so implementation-detailed, what do you think?

[aaemnnosttv]

@techanvil This is looking great, just one more thought and then I think we can move forward with this as an AC+IB as you suggested.

• Create a new error store selector, getSelectorDataForError which takes an error object as its argument.

I think we can simplify this to getArgsForError as the storage will be tied to receiveError. Given the direction here, I don't think we need to make this specific to selectors even if that's the only way it will likely be used.

Before the whole addition of selectorData, getErrorForSelector and getErrorForAction were purely semantic differences but the same thing under the hood.

I would suggest we keep the main lookup selector here agnostic to action/selector (similar to getError or getErrors) and if we later find that more scoping is needed, we can add type-specific selectors that do the cross-referencing with registered action/selector names.

[techanvil]

Thanks @aaemnnosttv. I have updated the AC to remove the selector-specific aspects. However, and sorry for yet more back and forth here, I have proposed the name getMetaDataForError rather than getArgsForError as we also need to return the baseName, as well as args. I've also added an IB which simply points to the AC.

[aaemnnosttv]

@techanvil ah of course, I forgot about the baseName 👍 That all sounds good, we just need an estimate here (as well as the test coverage section) and then this should be good to go!

[techanvil]

Thanks @aaemnnosttv - have updated accordingly and moved to IBR.

[aaemnnosttv]

IB ✅

[aaemnnosttv]

Agreed with QAB that it's not necessary here. Moving to Approval 👍