Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Removes instances of "any" and unnecessary type casts #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Removes instances of "any" and unnecessary type casts #236

wants to merge 2 commits into from

Conversation

mateja176
Copy link

No description provided.

@msftclas
Copy link

msftclas commented Oct 12, 2019

CLA assistant check
All CLA requirements met.

@mateja176 mateja176 changed the title Feat/remove instances of any Feat/remove instances of any and unnecessary type casts Oct 12, 2019
@mateja176 mateja176 changed the title Feat/remove instances of any and unnecessary type casts Removes instances of any and unnecessary type casts Oct 12, 2019
@mateja176 mateja176 changed the title Removes instances of any and unnecessary type casts Removes instances of "any" and unnecessary type casts Oct 12, 2019
Comment on lines +1 to +6
import { Request } from "express";
import { UserDocument } from "../models/User";

export interface Req extends Request {
user: UserDocument;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I''m not sure if that is the best approach. In TS one usually augments missing changes using the type definition file (declaration merging), as we don't own the Request type:
E.g., for the userproperty on the Request:
https://github.com/microsoft/TypeScript-Node-Starter/pull/227/files

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comment. Indeed, module augmentation may be the more idiomatic way to go about it, although I like the explicitness of wrapping ( extending ) a given type. Furthermore, it would be nice if the express's Request type were generic, similar to how RouteComponentProps accepts a parameter representing the type of the matched path params, however this is a topic for a different discussion. To get back to the main story line, why wasn't #227 merged yet ( #236 could build off of it )?

@GrayStrider
Copy link
Contributor

GrayStrider commented Oct 21, 2019

Seem to repeat many edits from #232 as well, we really need some merges or comments, @peterblazejewicz
Perhaps, move isKnownProvider into separate PR for review? @mateja176
It's a new feature and doesn't seem to do anything with any or typecasts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants