Properly type actions

[ivarnakken]

Description

Properly type the callAPI and promise middleware

It is now possible to type API request!

---

Type announcement, article, comment and company actions

Result

No visual changes except this tiny fix:

Before	After

Testing

• I have thoroughly tested my changes.

Tested all actions that were typed, even though that should not have broken anything.

[linear]

ABA-541 Type the call api

It works. Almost done 😬

[eikhr]

Hmm, there's something strange here. It seems like the provided T to callApi gets used to set the type of action.type, which is a string id (ish) for that specific action. But in the typed actions you provide the entity type fetched? This seems unintentional.

Also I think the Action-type you have changed here is used for other things beside just API-calls, meaning the types will be wrong. A new API-call specific action-type would probably be a good idea

[ivarnakken]

Hmm, there's something strange here. It seems like the provided T to callApi gets used to set the type of action.type, which is a string id (ish) for that specific action. But in the typed actions you provide the entity type fetched? This seems unintentional.

Yes, that's strange. The provided T is the type of the entities returned. It should be typed like this:

export type Action<T = unknown> = {
  type: unknown;
  payload?: {
    actionGrant?: ActionGrant;
    entities?: T;
    next?: string;
    previous?: string;
    result?: ID;
  };
};

---

Also I think the Action-type you have changed here is used for other things beside just API-calls, meaning the types will be wrong. A new API-call specific action-type would probably be a good idea

Not sure if I'm following you here. The Action type is only used to type actions?

[eikhr]

Not sure if I'm following you here. The Action type is only used to type actions?

Yes, but not only API-call actions. It is f.ex. used in ToastActions.ts for the removeToast action, which has a payload consisting of only an id. This doesn't fit your new Action type, but did fit the previous one which allowed for any payload.

I'm just saying you should take your new type, call it f.ex. APIAction and use it for all the API-actions, while letting other types of actions keep on using the old type.

[ivarnakken]

Not sure if I'm following you here. The Action type is only used to type actions?

Yes, but not only API-call actions. It is f.ex. used in ToastActions.ts for the removeToast action, which has a payload consisting of only an id. This doesn't fit your new Action type, but did fit the previous one which allowed for any payload.

I'm just saying you should take your new type, call it f.ex. APIAction and use it for all the API-actions, while letting other types of actions keep on using the old type.

Yes but all actions are currently wrong and have to be refactored.

This PR just makes it possible to actually type them

[eikhr]

Not sure if I'm following you here. The Action type is only used to type actions?

Yes, but not only API-call actions. It is f.ex. used in ToastActions.ts for the removeToast action, which has a payload consisting of only an id. This doesn't fit your new Action type, but did fit the previous one which allowed for any payload.
I'm just saying you should take your new type, call it f.ex. APIAction and use it for all the API-actions, while letting other types of actions keep on using the old type.

Yes but all actions are currently wrong and have to be refactored.

This PR just makes it possible to actually type them

The deleteToast action is perfectly fine? Why would it need entities in it, it simply removes the toast with the given id from the redux state

[eikhr]

Hmm, there's something strange here. It seems like the provided T to callApi gets used to set the type of action.type, which is a string id (ish) for that specific action. But in the typed actions you provide the entity type fetched? This seems unintentional.

Yes, that's strange. The provided T is the type of the entities returned. It should be typed like this:

export type Action<T = unknown> = {
  type: unknown;
  payload?: {
    actionGrant?: ActionGrant;
    entities?: T;
    next?: string;
    previous?: string;
    result?: ID;
  };
};

Regarding this part, I think you may have forgotten to push the latest changes? In the diff right now the action type looks like this:

export type Action<T = unknown> = {
  type: T;
  payload?: {
    actionGrant?: ActionGrant;
    entities?: Record<string, unknown>;
    next?: string;
    previous?: string;
    result?: ID;
  };
};

It should be more like what you posted over, except that entities is actually a map of entity type names and a map of entityId to the entity (this is because it is run through normalizr):

It you look in app/store/models/entities.ts I have actually created a helper type for this. NormalizedEntityPayload<EntityKeys> will give you the type of the entities-field containing all the entity types specified in EntityKeys.

[eikhr]

I think this works well now! I suggest we merge this, and then add types to the remaining actions as they are needed. They will default to unknown.

[eikhr]

I think this is good now!
But maybe someone else should review as well, as I wrote quite a bit of it:)

[ollfkaih]

LGTM