chore: Updated Typescript types.

[yaldram]

Description

• Update typescript to 4.2.4 & fix typescript types.

Fixes #11091
Fixes #9134

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• Run yarn start the code should compile without any typescript errors in the console.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/get-appsmith/appsmith/GhTvujQpg7ffVjTSGSkPHNv6j5KB
✅ Preview: https://appsmith-git-typescript-upgrade-get-appsmith.vercel.app

[arunvjn]

For such selectors or function where we know that return types cannot be undefined/null, we should wrap the return values of these functions/selectors with a function like below and reduce type.

function shouldBeDefined<T>(argument: T | undefined | null, message: string = 'some error message to default to'): T {
  if (argument === undefined || argument === null) {
    throw new TypeError(message);
  }

  return argument;
}

Here the return value of the getCurrentUser can be wrapped with shouldBeDefined to get the type we expect.

[yaldram]

Thanks for your suggestions. As discussed yesterday we have many selectors that use .find() and therefore return T | undefined. We can either wrap all our selectors with the above function or we can improve the types like making the actionId of type keyof typeof state.actions.

[arunvjn]

@yaldram That might not work since the return type of .find() itself, is T | undefined.
Also, we should only wrap the return where we are sure that it cannot be undefined.

[yaldram]

@arunvjn Thanks, yes we need to implement shouldbeDefined like solution. @riodeuno

[arunvjn]

We should write and use a utility type here like type MayNotExist<T> = T | undefined;

[yaldram]

Thanks make sense, we will first try to handle the T | undefined cases in the selectors if a selector is returning T | undefined I will replace all the above type with type MayNotExist<T> = T | undefined;

[arunvjn]

We need to standardize the names for request and response types. This needs to done across the code base. Naming convention needs to be discussed and decided. I vote for ${api}RequestData and ${api}ResponseData. So in this case, the types would be FetchFormConfigRequestData and FetchFormConfigResponseData

[yaldram]

Good suggestion. @riodeuno please provide your inputs.

[arunvjn]

Throw error here when pageName is not found. Makes sure the selector always return string. Refer
https://github.com/appsmithorg/appsmith/pull/11193/files#r811332675

[yaldram]

@arunvjn @riodeuno I am leaving behind some of my findings : -

1. I have implemented shouldBeDefined locally thanks to @arunvjn's suggestions a lot of the type issues were resolved.
2. But there are some types in our code which do return undefined and we have handled them in places and we have some type mismatches also, for now I have @ts-expect-error but we can resolve these issues by talking to the authors.
3. We should implement type MayNotExist<T> at many places in our code we have T | undefined (across 158 files).
4. We should also type the unknown Api Responses if possible.

[yaldram]

Got some answers for typing redux-saga - redux-saga/redux-saga#2251

[riodeuno]

@yaldram Could you explain why we have changed interface to type here?
The reason for asking, is that if we have a standard reason for deciding one over the other, we can make it a practice in all of our codebase.

[yaldram]

Actually we have our ApiResponse as a generic type here, before it was an interface so I have changed some interfaces to types like : -

type TemplateResponse = ApiResponse<Template>

and if you want to add more params to the type just extend it using & like -

type JSCollectionCreateUpdateResponse = ApiResponse & { id: string } 

[arunvjn]

All API responses are of the type ApiResponse. We are doing something wrong here.
Did you mean to do something like ApiResponse<Template & { id: string }> ?

[yaldram]

Hey @arunvjn no actually this id field of type string is an additional field on the response and not part of the data field.

[riodeuno]

I am wondering if this could be achieved by using interface extends ApiResponse.
@yaldram I believe we had discussed the advantage of this approach. Could you mention it here so that we can consider this consistently further into the future.

[yaldram]

I had a discussion with @arunvjn where he suggested to use ApiResponse<T> as a Generic type we had a GenericApiResponse previously but it had the same fields as ApiResponse with an additional field that can be undefined. We are now replacing all our extends ApiResponse to use the generic version of it.

old version : -

export type ApiResponse = {
  responseMeta: ResponseMeta;
  data: any;
  code?: string;
};

export interface ActionExecutionResponse {
  responseMeta: ResponseMeta;
  data: {
    body: Record<string, unknown> | string;
    headers: Record<string, string[]>;
    statusCode: string;
    isExecutionSuccess: boolean;
    request: ActionApiResponseReq;
    errorType?: string;
  };
  clientMeta: {
    duration: string;
    size: string;
  };
}

now : -

export type ApiResponse<T = unknown> = {
  responseMeta: ResponseMeta;
  data: T;
  code?: string;
};

export type ActionExecutionResponse = ApiResponse<{
  body: Record<string, unknown> | string;
  headers: Record<string, string[]>;
  statusCode: string;
  isExecutionSuccess: boolean;
  request: ActionApiResponseReq;
  errorType?: string;
}> & {
  clientMeta: {
    duration: string;
    size: string;
  };
};

[riodeuno]

Can you confirm if this is supposed to be something like ApiResponse<{id: string}> instead?

[yaldram]

Yes, it should be : -

export type JSCollectionCreateUpdateResponse = ApiResponse & {
  id: string;
}

The above type has all the fields of ApiResponse, plus the id: string field.
old version : -

export interface JSCollectionCreateUpdateResponse extends ApiResponse {
  id: string;
}

[arunvjn]

Yes, this should be ApiResponse<{id: string}>

[yaldram]

Hey @arunvjn actually we have an additional field here id of type string which will not be part of the data field

[github-actions]

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
appsmith	✅ Ready (Inspect)	Visit Preview	Apr 21, 2022 at 10:55AM (UTC)

[riodeuno]

@yaldram Shall we close this without deleting the branch in favour of #13195?

[yaldram]

@yaldram Shall we close this without deleting the branch in favour of #13195?

Sure we can.

[riodeuno]

Closing in favour of #13195. DONOT delete branch. @yaldram must be consulted before attempting to do so.