MetaMask / MetaMask/metamask-extension

Split out actions and background methods from the actions file

Open
#17,529 4 comments 0 reactions 0 assignees View on GitHub
MV3 team-extension-platform
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

### Description
The actions file is currently extremely verbose and contains a plethora of actions that aren't really actions at all. There are two different types of actions that are not necessary to be coupled with our redux implementation:

#### "fake" thunks
```js
export function setConnectedStatusPopoverHasBeenShown() {
return () => {
callBackgroundMethod('setConnectedStatusPopoverHasBeenShown', [], (err) => {
if (err) {
throw new Error(err.message);
}
});
};
}
```
in the above example this is a thunk, a function that returns a function that is anticipated to be invoked by redux when receiving it as an action call. It gets invoked with dispatch as the first argument and getState as the second. In the case above neither dispatch nor getState is required, and therefore this method *does not need to be a thunk*. It also **does not need to be dispatched** or mapped to dispatch in containers.

#### Not actions, but still in the actions file
```js
export async function setAlertEnabledness(alertId, enabledness) {
await submitRequestToBackground('setAlertEnabledness', [
alertId,
enabledness,
]);
}
```

This example doesn't pretend to be an action, other then being included in the actions file.

### Proposed Solution
To help differentiate between redux and background I'd like to create a separate folder in the ui main folder. This folder will be called background and will contain entries that correspond to the controller whose methods are interacted with. In addition an index file will be created that will export all members of the subfiles
```
|- ui
|- background
|- preferences.ts
|- metametrics.ts
```
We should also move the action-queue.ts file into this folder. It might make sense to nest the individual background methods into a folder:
```
|- ui
|- background
|- APIs
|- preferences.ts
|- metametrics.ts
|- index.ts
|- helpers
|- action-queue.ts
```
Methods that exist only on the metamask-controller, or that manipulate multiple controllers states can stay in the index.ts file inside the APIs folder.

Naming can be debated for these folders ,i'm not set on what is proposed here.

UPDATE:

I think it makes sense to have a folder for each Controller that the UI interacts with in this background folder. I do NOT think we should have a root level file that re exports things from these files. Instead we would import directly from the Controller folder. This would start to create relationships between UI files and the controllers upon which they rely/interact

### Benefits
1. Clearly distinguishing the background api from the redux actions demystifies the overlap between these two very different types of functions.
2. We can extract away all of the 'submitRequestToBackground' calls, and create typed APIs for these methods so that when we *do* use these in the actions file it is cleaner and further simplifies our actions.
3. Grouping background methods by controller seems like a good stepping stone to start seeing how all the different data flows. It also makes it very much easier for engineers to trace the flow of data.

#### Example usage:
```js
export function addPermittedAccount(origin, address) {
return async (dispatch) => {
await new Promise((resolve, reject) => {
callBackgroundMethod(
'addPermittedAccount',
[origin, address],
(error) => {
if (error) {
reject(error);
return;
}
resolve();
},
);
});
await forceUpdateMetamaskState(dispatch);
};
}
```

Could become something like this:

```
export function addPermittedAccount(origin, address) {
return async (dispatch) => {
await background.addPermittedAccount(origin, address);
await forceUpdateMetamaskState(dispatch);
}
```

We have now abstracted away how addPermittedAccount is implemented. If in the future we want to rewrite action-queue we can do seamlessly without the actions file being disturbed. We can also switch this method from using callBackgroundMethod to submitRequestToBackground which is the preferred way...as long as we return a promise the above snippet doesn't need to be modified.

#### Notes
This issue exists to start conversation and get buy-in. If the team is aligned it can be split up further:
1. Create folder structure for background folder (#17600)
2. Create typed methods for 'callBackgroundMethod' and 'submitRequestToBackground' instances in actions.ts for the preferences controller to the preferences.ts file in background/APIs/preferences.ts
3. ...Repeat for every distinct API file.
4. While we are doing the above we are *not* removing actions. So the `setConnectedStatusPopoverHasBeenShown` from the first example would still exist, but would only call `background.setConnectedStatusPopoverHasBeenShown`
5. Remove non redux actions, and also change any imports needed to import the specific background method from the background folder. So instead of importing `setConnectedStatusPopoverHasBeenShown` from the actions file, we import it from the background folder. If it was previously being dispatched we remove that element.

As you can see this task could easily be parallelized once an example is set and the folder structure is set up.

Contributor guide

Open the contributing guide

Research direction

Start by reading actions.ts and action-queue.ts, then review the proposed ui/background structure and the listed follow-up issues. This issue is exploratory rather than implementation-ready; done would require team agreement on the folder and API boundaries before the work is split into concrete tasks.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.