google / google/site-kit-wp

Remove sharing of referencing between preloaded fetch data and Redux.

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

Description

## Feature Description

We want to avoid exposing references accessible via the `window` or `global` object to data stored in Redux.

For an example of how data is currently shared:

![image.png](https://images.zenhubusercontent.com/6133937d41f397090b8344a7/9c02f138-adfe-49d6-a0eb-2ba188e62e67)

Note: We should be able to tackle this in a centralised manner, in [`createPreloadingMiddleware`](https://github.com/google/site-kit-wp/blob/fd764b7d56d11f9b571e719281a66bb044eb9cf0/assets/js/googlesitekit/api/middleware/preloading.js#L35).

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

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

## Acceptance criteria

* Data that Site Kit preloads into the page at load time is copied into the plugin's data store rather than shared with it.
* Changing a preloaded value on the page's global variables no longer changes the matching value held in the plugin's data store, and changing it in the store no longer changes the global.
* The copy covers values nested inside the preloaded data, not just the top level.
* Requests that were not preloaded still go to the server exactly as they do today.
* Every screen that relies on preloaded data loads with the same content, and no slower, than before.

## Implementation Brief

* [ ] Rename `assets/js/googlesitekit/api/middleware/preloading.js` to `assets/js/googlesitekit/api/middleware/preloading.ts`, converting it to TypeScript alongside `deduplication.ts`:
* [ ] Type the exported factory as `createPreloadingMiddleware( preloadedData: PreloadedData ): APIFetchMiddleware`, importing `APIFetchMiddleware` from `@wordpress/api-fetch`.
* [ ] Declare and export `PreloadedData` as a record keyed by path, whose values are either a response entry (`{ body: unknown; headers?: unknown }`) or, for the `OPTIONS` key, a nested record of such entries.
* [ ] Import `cloneDeep` from `lodash` under the External dependencies group and build `cache` from `cloneDeep( preloadedData[ path ] )`, so the cache holds no reference into `global._googlesitekitAPIFetchData.preloadedData` on either the `GET` body branch or the `OPTIONS` branch.
* [ ] Add `@since n.e.x.t Deep clones the preloaded data.` to the function's doc block.

* [ ] In `assets/js/api-fetch-shim.ts`:
* [ ] Drop the `as APIFetchMiddleware` cast and the `@TODO Remove this when createPreloadingMiddleware is properly typed.` comment on the `apiFetch.preloadingMiddleware` assignment, and update the import to `./googlesitekit/api/middleware/preloading`.

* [ ] In `assets/js/types/globals.d.ts`:
* [ ] Type `_googlesitekitAPIFetchData.preloadedData` as the `PreloadedData` type exported by the middleware.

### Test Coverage

* Rename `assets/js/googlesitekit/api/middleware/preloading.test.js` to `preloading.test.ts` and add cases:
* [ ] The resolved response for a preloaded `GET` path is not the same object as the entry's `body` in the data passed to the factory, and mutating the resolved response leaves that data unchanged.
* [ ] A nested object inside a resolved response is likewise not shared with the data passed to the factory.
* [ ] The response resolved for a preloaded `OPTIONS` path is not shared with the data passed to the factory.
* [ ] The existing equality, single-use, cache-expiry and `apiFetch` integration cases still hold.
* Add a case to `assets/js/googlesitekit/datastore/user/permissions.test.js`:
* [ ] Capabilities read back from the store are not the same object as `_googlesitekitAPIFetchData.preloadedData[ '/google-site-kit/v1/core/user/data/permissions' ].body`.

## QA Brief

*

## Changelog entry

*

Contributor guide

Open the contributing guide

Research direction

Start with assets/js/googlesitekit/api/middleware/preloading.js and its preloading.test.js, then inspect assets/js/api-fetch-shim.ts and assets/js/types/globals.d.ts. Run the existing middleware and permissions tests before making the TypeScript and cloning changes. Done means preloaded GET and OPTIONS data, including nested values, is independent from global data while non-preloaded requests and existing behavior remain unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript
Domain
api, testing
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.