Site-kit-wp: Implement more appropriate error handling in JS data stores and refactored components

[felixarntz]

Currently Site Kit data stores simply store the error in an error property per store which has a few drawbacks:

• Only one error per store is saved.
• It is unclear where the error came from.
• It is unclear whether that error should be displayed and overrule regular app flow or whether it can be ignored.

In components, we currently use an ErrorNotice component which simply takes that single error and displays it. While this works _fairly_ well for module setup flows and settings being edited, it will show more obvious disadvantages as we're migrating dashboard widgets (e.g. PageSpeed Insights widget right now doesn't display any alternative content on error, it just keeps loading). The goal here should be that every widget knows whether (one of) its API queries resulted in a failure and that it then returns appropriate error content. Eventually such errors should use predefined components so that e.g. the widget area renderer can detect that and aggregate certain types of errors together - this way users would see a single error message instead of e.g. 4 times the same one.

Components in general also need to have awareness of errors. For example, loading indicators shouldn't keep loading forever when there was actually an error that will prevent the loading process from being completed. Related example: https://github.com/google/site-kit-wp/commit/d256c1065d91172bd9615682936804e5ddd61f88

Let's think about how we can make errors available to components in a more granular way.

---

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

Acceptance criteria

Error Store

• There should be a createErrorStore() function which creates a partial store for errors. It should have:
  
  
  
  • action receiveError( error, baseName, args ) (should be marked as private)
    
    
    
    
    
    • Theoretically, all three parameters are required, however due to legacy support for now only error should be required.
    
    
    
    • baseName should be documented with something like "This should be a selector or action name."
    
    
    
    • with all arguments provided (will be required once legacy part below no longer exists), the reducer should key the error by baseName and args (only one error per combination, i.e. an error for the exact same request would override the previous one)
    
    
    
    • if only error is provided, just store it in error state property like today; this part of the reducer should be annotated as legacy behavior with a TODO comment to remove it once all instances of that behavior have been removed
    
    
    
    • no new code should ever pass error only
    
    
    
  
  
  • selector getError( baseName, args ) (should be marked as private)
    
    
    
    
    
    • Theoretically baseName should be required, but due to legacy support for now no parameters should be required.
    
    
    
    • baseName should be documented with something like "This should be a selector or action name."
    
    
    
    • if only baseName is provided, return the first error for that baseName (i.e. for any args)
    
    
    
    • if no parameters are provided, just look up in error state property like today; this part of the selector should be annotated as legacy behavior with a TODO comment to remove it once all instances of that behavior have been removed
    
    
    
    • no new code should ever call this without parameters
    
    
    
  
  
  • selector getErrorForSelector( selectorName, args )
    
    
    
    
    
    • just calls getError, the only difference is that here selectorName should actually be required (no legacy support needed), and this selector should be public
    
    
    
  
  
  • selector getErrorForAction( actionName, args )
    
    
    
    
    
    • just calls getError, the only difference is that here actionName should actually be required (no legacy support needed), and this selector should be public
    
    
    
  
  
  • selector getErrors( baseName ) (should be marked as private)
    
    
    
    
    
    • returns array of all errors that are stored for the baseName, or empty array
    
    
    
  
  
  • selector getErrorsForSelector( selectorName )
    
    
    
    
    
    • just calls getErrors
    
    
    
  
  
  • selector getErrorsForAction( actionName )
    
    
    
    
    
    • just calls getErrors
    
    
    
  
  
  • let's not do the above ones for now, we can introduce them if we need them at some point
  
  
• _Every_ store throughout the plugin should include this error partial store. Stores that currently have the basic getError or receiveError functions should have that part removed, the new store includes support for that.

Dispatching errors

• createFetchStore should dispatch API errors via the new receiveError, if available on the store (in our case it will be for every store, see above).
  
  
  
  • It should also have a secondary receiveError call with _only_ the error itself, to replicate what is currently hard-coded in the CATCH_FETCH reducer.
  
  
• For actions/selectors where the fetch store works slightly different from the action/selector itself, errors should be dispatched manually from the public action or resolver to match the public action/selector name/signature (in addition to the default handling within fetch store):
  
  
  
  • modules/analytics getAccounts (fetch store uses different name and arguments)
  
  
  • modules/analytics getProperties (fetch store uses different name)
  
  
  • createSettingsStore saveSettings (fetch store uses different arguments)
  
  

Handling errors

• The PageSpeed Insights dashboard widget should (basically as a first use-case) be modified to display errors for its API requests.
  
  
  
  • If the mobile request fails, the error should only surface in the mobile tab, and the same way for desktop.
  
  

Implementation Brief

In essence, the aim of this issue is to map errors to baseName + arguments. This is similar to how resolvers (and their "resolution" state) are mapped to selector name + arguments (see: https://github.com/google/site-kit-wp/blob/64b9b2b04349641f6b8b452267b4d4d59bfa3280/assets/js/googlesitekit/modules/datastore/modules.js#L251).

Currently we store errors encountered on a per-store basis, which means they aren't specific to particular actions/selectors. Dispatching errors for a particular set of arguments solves this problem.

One thing to consider is that we should _clear any errors_ for a given baseName + args combo whenever it is invoked again. Forcing the developer to do this manually is silly, so the aim should be that any action/resolver ahould clear its error before making a new "async" part of the request.

This means, in addition to the ACs mentioned (which act as IB), we should add:

• action: clearError( baseName, args ) (should be marked as private)
  
  
  
  • This will clear the existing error saved for this baseName + args combo so it no longer is returned by getErrorFor* selectors and doesn't appear in content.
  
  
• Add clearError( baseName, args ) to https://github.com/google/site-kit-wp/blob/ddde4fa52bcdebd93a9dd11784a7a6972144928d/assets/js/googlesitekit/data/create-fetch-store.js#L148 (after START_FETCH)
• Aside from that, I think the IB largely covers the intent of the issue aside from the particulars of where to put the calls, but I think they're evident from the ACs.
• Convert all existing errors to use our new createErrorStore actions, but use the private, basic getError and receieveError for now, with TODO: comments explaining the intent to convert them fully in a future issue. This cuts down on the amount of code to review.

QA Brief

• Test the PageSpeed Insights dashboard widget to ensure that errors encountered in the mobile tab appear in the mobile tab and ONLY the mobile tab. (Same goes for desktop.)

Changelog entry

• Introduce more granular error handling, with consistent error behavior in every store and API request errors being automatically stored.

[tofumatt]

Because the ACs here are largely something to follow for IB, I referenced them in the IB but added some context and also an extra clearError action to the IB. I think it's a good idea to integrate error clearing into our actions that dispatch errors so we don't wind up in a state where the error isn't cleared or needs to be cleared elsewhere.

[felixarntz]

IB ✅

[eugene-manuilov]

@tofumatt / @felixarntz IB says to add clearError action to the createFetchStore factory but how can i get to the clear function itself without knowning the current datastore name? Without it I can't call registry.dispatch( STORE_NAME ).clearError(...) sequence. Do I need to addd an additional parameter to the createFetchStore function to receive the current store name?

[felixarntz]

@eugene-manuilov Good point, yes createFetchStore should receive storeName then. Also, createFetchStore should check whether clearError (as well as the other error selectors/actions) exist on the store before calling them, since that is not a given.

[eugene-manuilov]

Created the #1864 PR with initial changes that include the createErrorStore partial store, changes in the createFetchStore, updates to existing stores and modification of the PSI dashboard widget to use the new approach to track and display errors. The PR itself seems to be pretty big, so I have decided to send it for review and process remaining tasks (if i miss anything) in separate PRs.

[felixarntz]

@eugene-manuilov I think you already covered everything in the PR overall, there are however a few things to fix. I think the only part that might have made sense to do separately is the PSI component changes, but since you have them there already, let's continue to cover everything in that PR.

[eugene-manuilov]

@felixarntz I have a question about Analytics' datastores. Do we really need to manually trigger receiveError actions? Error should be already processed by fetch stores and manually triggering receiveError means that we just generate duplicate actions. Am I right or do I miss something? :thinking:

[felixarntz]

@eugene-manuilov Yes we do. It might feel a bit odd from an engineering perspective, but the reason we need to do that in certain cases:

• The goal is that every error is stored under the baseName and args which are passed to the public action or selector.
• Through the automated handling in the fetch stores, errors are stored based on the baseName and args that the fetch store receives. These usually match the public action/selector, but in few cases _may_ be different.
• For most cases, this means the error is stored correctly. But for a few special cases, the fetch store uses a different baseName and args than the public action or selector that is using it. And the consuming code (e.g. the components) shouldn't need to know any internals about the action or selector's fetch mechanism to know where to look for the error.
  
  
  
  • We should be adding these "duplicate" calls (with different parameters though) only where the fetch action's baseName and args differ from the action/selector name and the action/selector args. Essentially, we're patching over these few inconsistencies.
  
  
  • It does mean that these errors will be in the datastore twice, but that doesn't matter - they'd be accessed through the baseName and args that we'd manually specify.
  
  
  • This isn't limited at all to Analytics, it just happens that the Analytics store has most of those cases for us.
  
  

The ACs have 3 of these cases (which probably is already most of such cases that we have at all), so you can look at those for examples: In each of them, something about the selector/action differs from the respective internal fetch action's signature.

[eugene-manuilov]

@felixarntz I have addressed your feedback in PR and I think it's ready for another review, but there are two components that rely on the legacy getError selector which I am not sure how to update:

https://github.com/google/site-kit-wp/blob/19a6a07dd97b0547b0644903c722e00ca0a3d8ab/assets/js/components/StoreErrorNotice.js#L32-L41

https://github.com/google/site-kit-wp/blob/19a6a07dd97b0547b0644903c722e00ca0a3d8ab/assets/js/modules/adsense/components/setup/SetupMain.js#L101

Do you know how to update it?

[felixarntz]

@eugene-manuilov I don't think we should update these components here? Also not mentioned in the ACs. This PR should only be about the datastore, except for the PSI module widget for which we should add error handling (like you've already done).

[eugene-manuilov]

@felixarntz you asked me to remove error from the CATCH_FETCH action and from the appropriate reducer: here and here. By doing it, we are losing the default getError() selector (without arguments) because error hanlder in the fetch store itself always sets errors using baseName and arguments list. So, we need to rework all legacy getError occurrences to use the new approach (with selector/action name and arguments list).

[felixarntz]

@eugene-manuilov I mentioned not only removing error from CATCH_FETCH, but also handling it via the receiveError action, see https://github.com/google/site-kit-wp/pull/1864#discussion_r465956464: I believe you didn't implement the second bullet point, that's how we'd have parameter-less getError working still.

[eugene-manuilov]

Thanks @felixarntz. I missed that part :doh:. Ok, updated it and it's ready for review. I have updated js tests to use new errors for selectors and actions, plus found some other places where we need to manually call receiveError. Also, added hasErrors selector as it was needed in one of tests.

[tofumatt]

Tested by simulating an error for the mobile selector and the desktop selector, and only the appropriate section had an error based on the error triggered. Here's a screencast of the mobile-only error being simulated, but both were tested:

QA, very nice ✅