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
Merged

chore(jest-resolve): improve types #10239

merged 1 commit into from Jul 5, 2020

Conversation

G-Rath
Copy link
Contributor

@G-Rath 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 馃殏

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

馃憤

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
if (this.requireStack?.length && this.requireStack.length > 1) {
if (this.requireStack?.length > 1) {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

> 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 {
Copy link
Member

Choose a reason for hiding this comment

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

nice solution

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

Choose a reason for hiding this comment

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

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

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 I realised this after I pushed. I'll restore the declare and just add & process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
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.

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

@G-Rath G-Rath requested a review from SimenB July 4, 2020 23:01
@SimenB SimenB merged commit 49de731 into jestjs:master Jul 5, 2020
@SimenB
Copy link
Member

SimenB commented Jul 5, 2020

Thanks!

@G-Rath G-Rath deleted the improve-jest-resolve-types branch July 5, 2020 09:11
@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.

None yet

3 participants