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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(jest-resolve): improve types #10239

Merged
merged 1 commit into from Jul 5, 2020

Conversation

@G-Rath
Copy link
Contributor

G-Rath commented Jul 4, 2020

Summary

Improves the types of jest-resolve mainly for #10177, and also cleaning up a few little nits around the place, including a nice little find in ModuleNotFoundError.

Test plan

Run the tests and see if anything breaks 馃殏

@G-Rath G-Rath force-pushed the G-Rath:improve-jest-resolve-types branch 3 times, most recently from 30aa229 to a502bc7 Jul 4, 2020
@@ -31,11 +31,11 @@ export default class ModuleNotFoundError extends Error {

let message = this._originalMessage;

if (this?.requireStack?.length && this!.requireStack!.length > 1) {
if (this.requireStack?.length && this.requireStack.length > 1) {

This comment has been minimized.

@SimenB

SimenB Jul 4, 2020 Collaborator

馃憤

This comment has been minimized.

@SimenB

SimenB Jul 4, 2020 Collaborator

Suggested change
if (this.requireStack?.length && this.requireStack.length > 1) {
if (this.requireStack?.length > 1) {

?

This comment has been minimized.

@G-Rath

G-Rath Jul 4, 2020 Author Contributor

> requires both sides to be a number, so you can't do undefined > 1 in TS - I thought about merging these but decided against it as I think the alternatives don't really add much while not being as clean, i.e (this.requireStack?.length ?? 0) > 1

@@ -21,11 +21,19 @@ type ResolverOptions = {
rootDir?: Config.Path;
};

declare global {

This comment has been minimized.

@SimenB

SimenB Jul 4, 2020 Collaborator

nice solution

namespace NodeJS {
export interface Process {
// "private" api
binding(type: string): {};

This comment has been minimized.

@SimenB

SimenB Jul 4, 2020 Collaborator

won't this pollute the global definition of all consumers as well? I'm down with adding pnp as optional, but this one is purposefully not added to global types by DT and is highly discouraged to use by Node itself

This comment has been minimized.

@G-Rath

G-Rath Jul 4, 2020 Author Contributor

Yeah I realised this after I pushed. I'll restore the declare and just add & process.

This comment has been minimized.

@G-Rath

G-Rath Jul 4, 2020 Author Contributor

fwiw this should be "fine" as it only takes effect when this file is imported, which none of the d.tss do since it's not a public function, but I'll change it for peace of mind all the same.

Copy link
Collaborator

SimenB left a comment

Left some q's, this is great, thanks!

@G-Rath G-Rath force-pushed the G-Rath:improve-jest-resolve-types branch from a502bc7 to b44017c Jul 4, 2020
@G-Rath G-Rath force-pushed the G-Rath:improve-jest-resolve-types branch from b44017c to 20325d0 Jul 4, 2020
@G-Rath G-Rath requested a review from SimenB Jul 4, 2020
@SimenB SimenB merged commit 49de731 into facebook:master Jul 5, 2020
21 of 22 checks passed
21 of 22 checks passed
cleanup-runs
Details
Running TypeScript compiler & ESLint
Details
Node v10.x on ubuntu-latest
Details
Node v10.x on macOS-latest
Details
Node v10.x on windows-latest
Details
Node v12.x on ubuntu-latest
Details
Node v12.x on macOS-latest
Details
Node v12.x on windows-latest
Details
Node v13.x on ubuntu-latest
Details
Node v13.x on macOS-latest
Details
Node v13.x on windows-latest
Details
Node v14.x on ubuntu-latest
Details
Node v14.x on macOS-latest
Details
Node v14.x on windows-latest Node v14.x on windows-latest
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-12 Your tests passed on CircleCI!
Details
ci/circleci: test-node-13 Your tests passed on CircleCI!
Details
ci/circleci: test-node-14 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
facebook.jest #20200704.10 succeeded
Details
@SimenB
Copy link
Collaborator

SimenB commented Jul 5, 2020

Thanks!

@G-Rath G-Rath deleted the G-Rath:improve-jest-resolve-types branch Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.