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.

Sign up for GitHub

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

Jump to bottom

chore: replace function types #10436

Merged
merged 4 commits into from
Sep 13, 2020
Merged

chore: replace function types #10436

merged 4 commits into from
Sep 13, 2020

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Aug 22, 2020

Summary

Relates to #10177

Test plan

See if CI passes

@SimenB
Copy link
Member

SimenB commented Aug 22, 2020

@G-Rath a single failure

G-Rath
G-Rath commented Aug 23, 2020
View reviewed changes
packages/jest-config/src/Deprecated.ts
@@ -26,7 +26,7 @@ export default {
Please update your configuration.`,

preprocessorIgnorePatterns: (options: {
preprocessorIgnorePatterns: Array<string>;
preprocessorIgnorePatterns?: Array<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.

While technically at runtime this isn't true (because whole logic is that these functions are being called when options contains the property they're the value of), given they're deprecated and this doesn't break anything, I think it's fine 🤷

@SimenB
Copy link
Member

SimenB commented Aug 25, 2020

@G-Rath still some type errors here

SimenB
SimenB reviewed Aug 25, 2020
View reviewed changes
packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts
@@ -5,6 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import type BabelTraverse from '@babel/traverse';
Copy link
Member

Choose a reason for hiding this comment

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

is this in package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup :)

SimenB
SimenB reviewed Aug 25, 2020
View reviewed changes
packages/jest-snapshot/src/State.ts Outdated
@@ -5,6 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import type BabelTraverse from '@babel/traverse';
Copy link
Member

Choose a reason for hiding this comment

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

is this in package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope :(

(which is odd because I know I checked this 🤔)

It's listed as a dev dependency

Copy link
Member

@SimenB SimenB Sep 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We need the types dep. For both I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, this comes from @types/babel__travese, so we should be good to add that as the dependency 🎉

@SimenB
Copy link
Member

SimenB commented Aug 25, 2020

Can this PR also remove the Function entry here?

https://github.com/facebook/jest/blob/326b165642a6c87c6ebfcbb10544389aac5412bb/.eslintrc.js#L27

@SimenB
Copy link
Member

SimenB commented Sep 6, 2020

@G-Rath ping 🙂

@G-Rath
Copy link
Contributor Author

G-Rath commented Sep 13, 2020

@SimenB this is failing due to

Got error Error: /home/runner/work/jest/jest/packages/jest-snapshot/build/State.d.ts has the following non-node type references:

/// <reference types="babel__traverse" />

Which I'm not entirely sure is incorrect in this situation.

@SimenB
Copy link
Member

SimenB commented Sep 13, 2020

Right, I've added some validation that the only /// <reference types is /// <reference types="node" /> as all others should be enough with normal import statements. I don't see why this import is changed into a reference one…

@G-Rath
Copy link
Contributor Author

G-Rath commented Sep 13, 2020

Yeah, I was wondering a similar thing - my current bet is that it's because jest-snapshot doesn't depend on @babel/traverse 🤔

But so then I'd expect the output from TS to be correct in this case, because we're only wanting the type.

@SimenB
Copy link
Member

SimenB commented Sep 13, 2020

@G-Rath pushed a variant that avoid the import statement

SimenB
SimenB reviewed Sep 13, 2020
View reviewed changes
packages/jest-validate/src/types.ts
@@ -11,7 +11,9 @@ type Title = {
warning?: string;
};

export type DeprecatedOptions = Record<string, Function>;
export type DeprecatedOptionFunc = (arg: Record<string, any>) => string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export type DeprecatedOptionFunc = (arg: Record<string, any>) => string;
export type DeprecatedOptionFunc = (arg: Record<string, unknown>) => 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.

iirc no because then TS knows too much and would force us to properly type the deprecated types too - I think a good way to make this change would be to do it after the latest deprecated options are removed, so to require all future deprecated options to be more accurately typed.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good 👍

@SimenB SimenB changed the title Replace function types choreØ eplace function types Sep 13, 2020
@SimenB SimenB merged commit 3dd4a95 into jestjs:master Sep 13, 2020
@G-Rath
Copy link
Contributor Author

G-Rath commented Sep 13, 2020

choreØ eplace function types

@SimenB er what happened there 😅

@G-Rath G-Rath deleted the replace-function-types branch September 13, 2020 08:48
@SimenB
Copy link
Member

SimenB commented Sep 13, 2020

no idea...

@SimenB SimenB changed the title choreØ eplace function types chore: replace function types Sep 13, 2020
@SimenB
Copy link
Member

SimenB commented Sep 13, 2020

too late for the commit, tho :P

This was referenced Nov 17, 2020
Open
Open
Open
Open
Open
Open
This was referenced Nov 18, 2020
Open
Open
Open
Open
Open
Open
Open
This was referenced Mar 28, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
@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.
Reviewers

@SimenB SimenB SimenB left review comments

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@G-Rath @SimenB @facebook-github-bot