Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve typescript types #40

Merged
merged 3 commits into from
Mar 9, 2021
Merged

Improve typescript types #40

merged 3 commits into from
Mar 9, 2021

Conversation

stuft2
Copy link
Contributor

@stuft2 stuft2 commented Mar 4, 2021

Problem

The OpenAPI Enforcer MiddlewareFunction and the Express RequestHandler / ErrorRequestHandler types are incompatible:

TS2345: Argument of type 'RequestHandler<ParamsDictionary, any, any, ParsedQs, Record<string, any>>' is not assignable to parameter of type 'MiddlewareFunction'.   Types of parameters 'req' and 'req' are incompatible.     Type '{}' is missing the following properties from type 'Request<ParamsDictionary, any, any, ParsedQs, Record<string, any>>': get, header, accepts, acceptsCharsets, and 77 more.

TS2322: Type 'ErrorRequestHandler >' is not assignable to type 'MiddlewareFunction'.

Reproduction

Typescript v4.2.2

import {RequestHandler, ErrorRequestHandler} from 'express'
import {MiddlewareFunction} from 'openapi-enforcer-middleware'

const expressMiddleware: RequestHandler = (req, res, next) => {}
const expressErrMiddleware: ErrorRequestHandler = (err, req, res, next) => {}

const enforcerMiddleware: MiddlewareFunction = expressMiddleware // TS2345 error occurs
const enfrorcerErrMiddleware: MiddlewareFunction = expressErrMiddleware // TS2322 error occurs

Solution Description

  • Utilize the @types/express type definitions instead of similar custom types

If the Enforcer Middleware is built for Express, then we can safely assume the RequestHandler and ErrorRequestHandler types should be used instead of custom types.

Additional updates

  • Improve the type inference of injected dependencies (see Tuples in rest parameters and spread expressions)
  • Pass along the NextFunction type from express instead of a similar custom type
  • Implements Controllers and ControllersMap types to avoid redundant declarations of Middleware and Controllers dictionaries

These updates remove most of the usages of the object type to narrow what types are and are not allowed. Additionally, the use of object is discouraged since it's usage is finicky. For example: ESLint bans Object and object types by default citing this Typescript Issue.

index.d.ts Outdated
controllers (controllersDirectoryPath: string|object, ...dependencyInjection: any): Promise<object>;
middleware (): RequestHandler;
mocks (controllersDirectoryPath: string|object|undefined, automatic?: boolean, ...dependencyInjection: any): Promise<object>;
controllers<T extends unknown[]> (controllersDirectoryPath: string | OpenApiEnforcerMiddleware.Controllers, ...dependencyInjection: T): Promise<object>;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please educate me on this one. Why <T extends unknown[]> instead of using any[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer is that any isn't type safe so I always default to using unknown instead of any. It's kind of like using const by default even though you could use let to declare all variables in javascript.

You can read more about unknown vs any on this stackoverflow answer or on the Typescript 3.0 RC.

@Gi60s
Copy link
Owner

Gi60s commented Mar 5, 2021

Overall I love this PR, just that one question that I listed as a comment. Let me know the answer and I'll work on publishing this. Thank you.

@Gi60s Gi60s merged commit ef659ee into Gi60s:master Mar 9, 2021
@Gi60s
Copy link
Owner

Gi60s commented Mar 9, 2021

Published to NPM as version 1.2.4.

@stuft2 stuft2 deleted the types branch March 10, 2021 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants