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

Fix relative path #57961

Closed
wants to merge 2 commits into from
Closed

Conversation

xlianghang
Copy link

@xlianghang xlianghang commented Mar 27, 2024

Fixes #57956
Fixes #57926

Related changes: fbcdb8c#r140302331

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 27, 2024
Comment on lines +551 to +552
const nearestTargetPackageJson = getNearestAncestorDirectoryWithPackageJson(host, getDirectoryPath(modulePath), { returnAsBooleanIfHostMethodMissing: true });
const nearestSourcePackageJson = getNearestAncestorDirectoryWithPackageJson(host, sourceDirectory, { returnAsBooleanIfHostMethodMissing: true });
Copy link
Member

Choose a reason for hiding this comment

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

Why not just do !! at the start of these? No other calls need to pass these options in.

Copy link
Author

Choose a reason for hiding this comment

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

Why not just do !! at the start of these? No other calls need to pass these options in.

According to the logic before, if the host.getNearestAncestorDirectoryWithPackageJson methods exist, will use the return value of a string

Copy link
Member

Choose a reason for hiding this comment

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

I see, didn't realize this returned a string, and the point is to compare those.

I don't think that this is exactly how we'd want to fix it as it seems to overfit for the bug, but without a test case I can't really offer a better idea yet.

Copy link
Member

Choose a reason for hiding this comment

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

I'll say that this fix just seems confusing; it's basically changing the code to report maybeNonRelative if the paths have differing "is in a package"-ness, but that sure feels like the wrong signal. But that's what the old code kinda did too. @andrewbranch may have a better clue here.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't you just be able to check that getNearestAncestorDirectoryWithPackageJson(host, sourceDirectory) !== undefined?

@jakebailey
Copy link
Member

FWIW you'll want to pull out a test for this; since you made a minimal repro in #57956 you should be able to construct a test case in tests/cases/compiler with multiple files and declaration emit enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
4 participants