google / google/site-kit-wp

Enforce consistent selector resolution and fetch error keys in datastore resolvers

Open
#13,586 0 comments 0 reactions 0 assignees View on GitHub
Type: Bug Type: Enhancement
Dominant language
JavaScript
Stars
1.4k
Forks
383
Avg merge
4d 14h
Merged PRs (30d)
77

Description

## Feature Description

Site Kit datastore resolvers have an implicit contract with their corresponding selectors and fetch actions: the selector resolution and any selector error produced by the fetch must use the same argument list.

`@wordpress/data` tracks resolver state using the arguments passed to the selector. Separately, `createFetchStore` associates fetch errors with the arguments passed to the generated fetch action. Components such as `ErrorNotice` and `ReportErrorActions` use the fetch error’s stored arguments to invalidate the corresponding selector resolution when the user retries.

If a resolver defaults, normalizes, transforms, adds, or removes arguments before calling its fetch action, these two keys can differ. Retry then invalidates a resolution that the consuming component is not selecting, so the resolver does not run again.

For example:

```js
const resolvers = {
*getItems( params = {} ) {
yield fetchGetItems( params );
},
};
```

Calling:

```js
select( STORE ).getItems();
```

creates the selector resolution:

```js
getItems, []
```

However, a failed request is associated with:

```js
getItems, [ {} ]
```

Retry invalidates `[{}]`, while the component continues selecting `[]`.

#13587 addresses this specific problem for the Reader Revenue Manager `getPublication` selector but ideally this would be addressed in a way where the problem is handled for all resolvers at a higher level or automatically caught by some sort of linting or test.

Some existing code already accounts for this contract explicitly, including the PageSpeed Insights `getReport` resolver and the shared `createGetReportResolver` but the following resolvers appear to have similar risks and should be checked:

- `assets/js/modules/reader-revenue-manager/datastore/ctas.ts`
- `getCTAs( params = {} )` forwards the default object to both the selector and `fetchGetCTAs`.
- A call to `getCTAs()` may therefore use `[]` for its resolution and `[{}]` for its error.
- `assets/js/googlesitekit/datastore/site/email-reporting.js`
- `getEligibleSubscribers( eligibleSubscribersArgs = {} )`
- `getSubscribedUsers( subscribedUsersArgs = {} )`
- Both normalize the original selector arguments before calling their selectors and fetch actions. For example, `{ search: '' }` can become `{ page: 1, search: '' }`, producing different resolution and error keys.
- These resolvers also perform additional paginated requests. Errors from later-page requests may be associated with argument sets that were never used to invoke the resolver directly.

This list is not necessarily exhaustive.

---------------

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

## Acceptance criteria

*

## Implementation Brief

* [ ]

### Test Coverage

*

## QA Brief

*

## Changelog entry

*

Contributor guide

Open the contributing guide

Research direction

Start by comparing selector and fetch-action arguments in assets/js/modules/reader-revenue-manager/datastore/ctas.ts and assets/js/googlesitekit/datastore/site/email-reporting.js, then review the related #13587 change and existing PageSpeed Insights resolver patterns. Done means the affected resolvers keep resolution and fetch error keys aligned, with coverage for defaulted, normalized, and paginated arguments.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
backend-api-design, developer-experience
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.