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

General Type Maintenance #8462

Merged
merged 5 commits into from
May 15, 2019
Merged

Conversation

JoshRosenstein
Copy link
Contributor

Summary

As part of #8436 , this PR has a much larger scope, targeting obvious unnecessary castings and consistent Object Literal Types throughout the codebase.

For consistency I decided to use Record<string,string> if the type didnt have a meaningful index key ie:

type PathLookup= {[path:string]:string}
--- type LookupObj= {[key:string]:string}
+++ type LookupObj= Record<string,string>

Test plan

@SimenB SimenB requested a review from jeysal May 14, 2019 06:44
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Yay, this is awesome!

template: Template | any,
[head, ...tail]: Array<string>,
): any => {
const getPath = (template: Template, [head, ...tail]: Array<string>): any => {
Copy link
Member

Choose a reason for hiding this comment

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

any way to have it not return any? unknown is preferred

Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't it return Template | undefined?

Copy link
Contributor Author

@JoshRosenstein JoshRosenstein May 14, 2019

Choose a reason for hiding this comment

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

I will update this with an overload to return the actual nested type, and unknown if it cant determine. I will overload up to 5 nested levels.

const test2 = getPath({a: {b: 2}}, ['a']); // return type:{b:number}
const test3 = getPath({a: {b: 2}}, ['a', 'b']); // return type:number
const test4 = getPath({a: {b: 2}}, ['a', 'bc']); // return type:unknown
const test5 = getPath({}, ['a']); // return type:unknown

packages/jest-haste-map/src/ModuleMap.ts Show resolved Hide resolved
@@ -176,7 +176,7 @@ export function printObjectProperties(
printer: Printer,
): string {
let result = '';
const keys = getKeysOfEnumerableProperties(val);
const keys = getKeysOfEnumerableProperties(val) as Array<string>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this need to be casted, provided that it's already done in line 22? Maybe declaring the return type for getKeysOfEnumerableProperties would help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops first added it here, then decided to change it in getKeysOfEnumerableProperties, and forgot to take this one off. Thanks!, BTW- this functions is duplicated a few times throughout, maybe we should move this in jest-utils in different PR?

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

nice cleanup!

@codecov-io
Copy link

codecov-io commented May 14, 2019

Codecov Report

Merging #8462 into master will decrease coverage by <.01%.
The diff coverage is 68.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8462      +/-   ##
==========================================
- Coverage   62.32%   62.31%   -0.01%     
==========================================
  Files         266      266              
  Lines       10735    10735              
  Branches     2614     2617       +3     
==========================================
- Hits         6691     6690       -1     
  Misses       3460     3460              
- Partials      584      585       +1
Impacted Files Coverage Δ
packages/expect/src/asymmetricMatchers.ts 82.29% <ø> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/createSpy.ts 0% <ø> (ø) ⬆️
packages/jest-validate/src/warnings.ts 87.5% <ø> (ø) ⬆️
packages/jest-resolve/src/index.ts 44.52% <ø> (ø) ⬆️
packages/jest-core/src/FailedTestsCache.ts 76.47% <ø> (ø) ⬆️
packages/jest-config/src/setFromArgv.ts 86.36% <ø> (ø) ⬆️
packages/jest-snapshot/src/utils.ts 92.42% <ø> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/spyRegistry.ts 0% <ø> (ø) ⬆️
packages/jest-config/src/utils.ts 75.86% <ø> (ø) ⬆️
packages/jest-worker/src/Farm.ts 100% <ø> (ø) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15e2cc6...6bd1ad8. Read the comment docs.

@SimenB SimenB merged commit ccea646 into jestjs:master May 15, 2019
@JoshRosenstein JoshRosenstein deleted the general-type-cleanup branch May 15, 2019 13:30
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants