-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Sort the paths for module specifier by closeness to importing file path #33567
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make sense to do this in getNewImportInfos
(importFixes.ts) too? It currently sorts by string length, which feels kind of arbitrary.
@andrewbranch Not sure if its needed or not, since that kind of also iterates over result from different symbols.. |
a73ea2c
to
220ee81
Compare
9cce6d0
to
382ff17
Compare
@@ -57,5 +57,8 @@ Object.defineProperty(exports, "__esModule", { value: true }); | |||
__export(require("./keys")); | |||
|
|||
|
|||
//// [keys.d.ts] | |||
import { MetadataAccessor } from "@raymondfeng/pkg2"; | |||
export declare const ADMIN: MetadataAccessor<boolean, import("../../pkg1/dist").IdType>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is better or not..
cc: @weswigham @DanielRosenwasser @RyanCavanaugh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably not... I mean, technically it's correct, but when your code is seen in a structure like this (monorepo-y), you're not supposed to access sibling packages with a relative path, since the packages won't be siblings when shipped... The error was good in this case, since it called out that your types depended on a package you didn't directly depend on, which meant you needed to fix it in some way. Like this, it's.... Eck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out we resolve that only because of how these tests run (all the sources are included irrespective of tsconfig file) when I add test using the tsc baseline I retain the error so should be ok? appending that test as a separate commit for reference 91c66a0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping So this PR is good to go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, yeah.
/lib/lib.d.ts | ||
/src/plugin-one/node_modules/typescript-fsa/index.d.ts | ||
/src/plugin-one/action.ts | ||
/src/plugin-one/node_modules/plugin-two/node_modules/typescript-fsa/index.d.ts -> /src/plugin-one/node_modules/typescript-fsa/index.d.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I didn't notice it before, but isn't listFiles
consumed by some VS MSBuild API, and isn't this chance to include mappings with ->
probably going to break it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc ... @uniqueiniquity ?
Apparently the bug still occurs. See #32970. |
Fixes #32970
This uses #33566 to baseline
tsc --d
Also added way to show redirects when listing files..