gtap-dev / javascript

JavaScript Style Guide
MIT License
0 stars 0 forks source link

30.1 - alternative for explicit types everywhere #8

Open mjomble opened 3 years ago

mjomble commented 3 years ago

The TypeScript ESLint rule that we use to enforce this actually recommends not using itself 😄

Note: requiring type annotations unnecessarily can be cumbersome to maintain and generally reduces code readability. TypeScript is often better at inferring types than easily written type annotations would allow.

Instead of enabling typedef, it is generally recommended to use the --noImplicitAny and --strictPropertyInitialization compiler options to enforce type annotations only when useful.

Although this affects other cases as well, I'm initally focusing on functions.

I agree with adding explicit type definitions on the parameters and of standalone functions, e.g.

export const foo = (bar: string, baz: number): boolean => { ...

And even though their return types can usually be inferred, declaring them explicitly tends to make type errors clearer and might also improve TypeScript performance.

However, explicit type annotations seem unnecessary and annoying to maintain when a function is part of a larger expression where the types are unambiguous from the context.

For example, if we have a function like this:

const registerHandler = (
  urlPattern: string,
  callback: (
    method: HttpMethod,
    urlParams: URLSearchParams,
    req: IncomingMessage,
    requestId: string,
  ) => Promise<HttpResponse | undefined>,
) => {
  ...
};

Then without requiring explicit types everywhere, we could do this:

registerHandler('/foo', async (method) => {
  ...
});

registerHandler('/bar/:id', async (method, params, req, res) => { 
  ...
});

And the types of the params, as well as the expected return types would be safely inferred.

But with this rule, we're forced to do this instead:

registerHandler('/foo', async (method: HttpMethod): Promise<HttpResponse | undefined> => {
  ...
});

registerHandler('/bar/:id', async (
  method: HttpMethod, 
  params: URLSearchParams, 
  req: IncomingMessage, 
  res: ServerResponse,
): Promise<HttpResponse | undefined> => {
  ...
});

Although there can be some benefits to this (similar to return types on standalone functions), in my opinion it hurts readability and productivity enough to outweigh them.

In addition to the recommended TypeScript settings, I recommend using the 'strict' option which enables these and several other rules that enforce code correctness.

mihkeleidast commented 2 years ago

I'm OK with relaxing this rule. Stricter type definitions can still be opt-in, if something somewhere is so important it is deemed necessary.

I think the main point for this rule is that inferring too many types throughout a codebase can make it too easy to make mistakes and mismatch types from real data. But I really haven't seen too much of this happening in real life scenarios.

mjomble commented 2 months ago

There is one issue I've often run into when omitting return types from functions, but there's an ESLint plugin that can catch this specific case: https://github.com/bytescale/eslint-plugin-return-types-object-literals

In addition to excess property checking, return types are also needed in these cases to link the properties to their type definitions.

I will make a PR that recommends this rule instead of the typedef rule.