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

chore: replace anys with unknowns #9626

Merged
merged 9 commits into from
Apr 5, 2020
Merged

chore: replace anys with unknowns #9626

merged 9 commits into from
Apr 5, 2020

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Feb 29, 2020

Summary

This replaces some of the any types in the codebase with either unknown or a more accurate type.
Also removes some casts in favor of using the generic parameter when calling `reduce.

Test plan

Make a PR, and see if it blows up :D

@@ -18,6 +18,7 @@ const ltrimNewlineRe = /^(\r?\n)+/;
const multilineRe = /(?:^|\r?\n) *(@[^\r\n]*?) *\r?\n *(?![^@\r\n]*\/\/[^]*)([^@\r\n\s][^@\r\n]+?) *\r?\n/g;
const propertyRe = /(?:^|\r?\n) *@(\S+) *([^\r\n]*)/g;
const stringStartRe = /(\r?\n|^) *\* ?/g;
const STRING_ARRAY: Array<string> = [];
Copy link
Contributor Author

@G-Rath G-Rath Feb 29, 2020

Choose a reason for hiding this comment

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

This in theory is more performant as it's not allocating a new array every time, but mainly it just lets us drop as Array<string> in two places

name === 'NamedNodeMap'
? Array.prototype.reduce.call(collection, propsReducer, {})
isNamedNodeMap(collection)
? Array.from(collection).reduce<Record<string, string>>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I inlined this function as there's no way to pass a generic parameter otherwise; it's inlined in DOMElement, and so now actually matches 1:1.

props[attribute.name] = attribute.value;
return props;
}, {} as 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.

This could actually be classed as a bug - it might not ever be reachable due to surrounding code, but still - it was an array when it should have been an object 🤷‍♂

@codecov-io
Copy link

codecov-io commented Feb 29, 2020

Codecov Report

Merging #9626 into master will increase coverage by 0.00%.
The diff coverage is 79.31%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9626   +/-   ##
=======================================
  Coverage   64.90%   64.90%           
=======================================
  Files         288      288           
  Lines       12195    12197    +2     
  Branches     3022     3022           
=======================================
+ Hits         7915     7917    +2     
  Misses       3639     3639           
  Partials      641      641           
Impacted Files Coverage Δ
packages/expect/src/matchers.ts 96.86% <ø> (ø)
packages/expect/src/toThrowMatchers.ts 86.48% <ø> (ø)
...ckages/jest-config/src/ReporterValidationErrors.ts 11.76% <ø> (ø)
packages/jest-config/src/normalize.ts 76.66% <ø> (ø)
packages/jest-core/src/FailedTestsCache.ts 76.47% <ø> (ø)
packages/jest-fake-timers/src/jestFakeTimers.ts 90.35% <ø> (ø)
packages/jest-jasmine2/src/jasmine/Env.ts 0.00% <ø> (ø)
packages/jest-jasmine2/src/jasmine/spyRegistry.ts 0.00% <ø> (ø)
packages/jest-resolve/src/nodeModulesPaths.ts 85.71% <ø> (ø)
packages/pretty-format/src/collections.ts 98.48% <ø> (ø)
... and 10 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 26951bd...4118c8d. Read the comment docs.

@G-Rath
Copy link
Contributor Author

G-Rath commented Feb 29, 2020

ci/circleci: test-browser:

error https://registry.yarnpkg.com/path-exists/-/path-exists-3.0.0.tgz: Extracting tar content of undefined failed, the file appears to be corrupt: "ENOSPC: no space left on device, write"

😬

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 13, 2020

Failing test seems to be unrelated - it's failing on a different worker related test on each action

toBeInstanceOf(this: MatcherState, received: any, expected: Function) {
toBeInstanceOf(
this: MatcherState,
received: new () => unknown,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these are safe, I'd prefer to use unknown and narrow down. We're supposed to throw if they are called with the wrong type anyways, and making those checks narrow the type seems ideal. These get called by user code who provides whatever they feel like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're totally right!

@@ -39,7 +39,7 @@ const formatErrorMsg = (domain: string, usage?: string) => {
return (msg: string) => domain + ' : ' + msg + usageDefinition;
};

function isSpy(putativeSpy: any) {
function isSpy(putativeSpy: Spy) {
Copy link
Member

Choose a reason for hiding this comment

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

this is supposed to be a typeguard, so maybe

Suggested change
function isSpy(putativeSpy: Spy) {
function isSpy(putativeSpy: unknown): putativeSpy is Spy {

?

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.

Thanks! Sorry about the slow response, been swamped lately.

I think most of these changes are great, but the changes in expect are not necessarily right I think (although they're not less right than any, but any is more honest)

@G-Rath G-Rath requested a review from SimenB April 5, 2020 01:47
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.

thanks!

@SimenB SimenB changed the title Replace anys with unknowns chore: replace anys with unknowns Apr 5, 2020
@SimenB SimenB merged commit 8b8f90a into jestjs:master Apr 5, 2020
jeysal added a commit to mmkal/jest that referenced this pull request Apr 10, 2020
…pshots

* upstream/master: (225 commits)
  docs: add CLA link to contributing docs (jestjs#9789)
  chore: roll new version of docs
  v25.3.0
  chore: update changelog for release
  chore(jest-types): correct type testRegex for ProjectConfig (jestjs#9780)
  feat(circus): enable writing async test event handlers (jestjs#9397)
  feat: enable all babel syntax plugins (jestjs#9774)
  chore: add helper for getting Jest's config in e2e tests (jestjs#9770)
  feat: pass ESM options to transformers (jestjs#9597)
  chore: replace `any`s with `unknown`s (jestjs#9626)
  feat: pass ESM options to Babel (jestjs#9766)
  chore(website): add copy button the code blocks (jestjs#9750)
  chore: bump istanbul-reports for new uncovered lines design (jestjs#9758)
  chore: correct CHANGELOG.md (jestjs#9763)
  chore(jest-types): expose type `CacheKeyOptions` for `getCacheK… (jestjs#9762)
  docs: Fix simple typo, seperated -> separated (jestjs#9760)
  v25.2.7
  chore: update changelog for release
  fix: drop getters and setters when diffing objects for error (jestjs#9757)
  chore(jest-types): correct return type of shouldRunTestSuite fo… (jestjs#9753)
  ...
@sandersn
Copy link

Note: this breaks a couple of tests in @types/jest's test suite:

expect.addSnapshotSerializer({
    print: (value: {}) => '',
    test: (value: {}) => value === value,
});

expect.addSnapshotSerializer({
    print: (value: {}, serialize: (val: {}) => string, indent: (str: string) => string, opts: {}) => '',
    test: (value: {}) => value === value,
});

unknown is not assignable to {}, so the first value now has to have the type unknown instead:

expect.addSnapshotSerializer({
    print: (value: unknown) => '',
    test: (value: {}) => value === value,
});

expect.addSnapshotSerializer({
    print: (value: unknown, serialize: (val: {}) => string, indent: (str: string) => string, opts: {}) => '',
    test: (value: {}) => value === value,
});

No idea if this example is realistic or not, I just noticed it while running the TS team's Definitely Typed tests. It seems reasonable that a serialiser should handle any JS value, including null and undefined.

@G-Rath G-Rath deleted the replace-anys-with-unknowns branch April 16, 2020 00:36
@SimenB
Copy link
Member

SimenB commented Apr 16, 2020

I think it makes sense. Ideally test would work as a typeguard for print, but I'm not sure how to achieve that?

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 16, 2020

I think it makes sense.

if it doesn't then the type should be object or narrower ;)

Ideally test would work as a typeguard for print

If I understand correctly:

type Serializer<T> = {
  print: (val: T, serialize: JestSerialize, indent: number) => string;
  test: (val: unknown) => val is T;
};

I can't find the actual places in code where this is used, but that should do the trick if you're just doing:

if(serializer.test(val)) {
  serializer.print(val, /* ... everything else ... */);
}

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 16, 2020

Oh also rereading @sandersn's example I think while yes the type is more accurate now, the value parameter should have the same type for both functions

i.e

// incorrect
expect.addSnapshotSerializer({
    print: (value: unknown) => '',
    test: (value: {}) => value === value,
});

// accurate
expect.addSnapshotSerializer({
    print: (value: unknown) => '',
    test: (value: unknown) => value === value,
});

// spot on
expect.addSnapshotSerializer({
    print: (value: MyType) => '',
    test: (value: unknown): value is MyType => value === value,
});

@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