chore(types): add type information #791
Conversation
There was a problem hiding this comment.
Just my 2 cents on this :)
Seems reasonable to me to remove the type when:
- the name of the variable is clear:
packageJSONPath - the type of the assigned value is clear:
let hasProp = false; - we use a common function:
const filePaths = args.slice(3);
I would keep the type when:
- we use a custom function:
const transformations = mapOptionsToTransform(config); - we initialize a variable using another variable:
const temp = action;
packages/generators/add-generator.ts
Outdated
| @@ -73,7 +72,7 @@ const traverseAndGetProperties = (arr: object[], prop: string): boolean => { | |||
| const searchProps = (answers: object, input: string): Promise<string[]> => { | |||
| input = input || ""; | |||
| return Promise.resolve( | |||
| PROPS.filter((prop: string): boolean => | |||
| PROPS.filter((prop): boolean => | |||
There was a problem hiding this comment.
In other occurrences of .filter you are keeping the type of the parameter. I would keep here too
There was a problem hiding this comment.
You are correct here. we need to have it here.
| @@ -162,7 +161,7 @@ export default class AddGenerator extends Generator { | |||
| this.configuration.config.item = action; | |||
| }); | |||
| } | |||
| const temp: string = action; | |||
| const temp = action; | |||
There was a problem hiding this comment.
It doesn't look obvious to me. I would keep the type here
There was a problem hiding this comment.
action is already a string, so temp inherits its type
|
|
||
| webpackConfig.forEach((scaffoldPiece: string): Promise<void> => { | ||
| const config: IConfiguration = webpackProperties[scaffoldPiece]; | ||
| const transformations: string[] = mapOptionsToTransform(config); | ||
| const transformations = mapOptionsToTransform(config); |
There was a problem hiding this comment.
It doesn't look obvious to me what's the return type of mapOptionsToTransform
There was a problem hiding this comment.
My inspiration for this and the above one is from the Typescript core repo. Again here the type is obvious if you know the method signature or the variable and we don't need to be verbose.
But well, I agree it is highly subjective...
There was a problem hiding this comment.
Actually there's no need to assign a type if the function already has a signature (and just one, where it returns always a string).
|
There were the following issues with this Pull Request
You may need to change the commit messages to comply with the repository contributing guidelines. 🤖 This comment was generated by commitlint[bot]. Please report issues here. Happy coding! |
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
| @@ -230,7 +237,7 @@ function addOrUpdateConfigObject( | |||
|
|
|||
| if (propertyExists) { | |||
| rootNode.properties | |||
| .filter((path: INode) => path.key.name === configProperty) | |||
| .filter((path: INode): boolean => path.key.name === configProperty) | |||
There was a problem hiding this comment.
Is it needed the type here?
|
Needs a quick rebase @sendilkumarn |
chore(types): fix lint issue chore(types): add missing types
webpack-bot
added
PR: review-outdated
and removed
PR: conflict
PR: reviewed-approved
labels
|
@sendilkumarn Thanks for your update. I labeled the Pull Request so reviewers will review it again. @evenstensberg Please review the new changes. |
|
Done rebasing, let us merge this once the builds are green 🎉 |
What kind of change does this PR introduce?
Adds some type related information and removed where it is obvious
Did you add tests for your changes?
N/A
If relevant, did you update the documentation?
N/A
Summary
leverage on type inference
r? @evenstensberg @ematipico @dhruvdutt
Does this PR introduce a breaking change?
Other information