-
Notifications
You must be signed in to change notification settings - Fork 655
Implement tryLoadInputFileForPath #1302
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,21 +5,19 @@ | |
- | ||
- | ||
-!!! error TS2209: The project root is ambiguous, but is required to resolve export map entry '.' in file 'package.json'. Supply the `rootDir` compiler option to disambiguate. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is missing because we currently aren't collecting resolution diagnostics (nor any program diagnostic). |
||
+index.ts(1,21): error TS2307: Cannot find module '@this/package' or its corresponding type declarations. | ||
+ | ||
+ | ||
==== package.json (0 errors) ==== | ||
{ | ||
"name": "@this/package", | ||
@@= skipped -9, +8 lines =@@ | ||
".": "./dist/index.js" | ||
} | ||
} | ||
-==== package.json (0 errors) ==== | ||
- { | ||
- "name": "@this/package", | ||
- "type": "module", | ||
- "exports": { | ||
- ".": "./dist/index.js" | ||
- } | ||
- } | ||
-==== index.ts (0 errors) ==== | ||
+==== index.ts (1 errors) ==== | ||
import * as me from "@this/package"; | ||
+ ~~~~~~~~~~~~~~~ | ||
+!!! error TS2307: Cannot find module '@this/package' or its corresponding type declarations. | ||
me.thing(); | ||
- import * as me from "@this/package"; | ||
- | ||
- me.thing(); | ||
- | ||
- export function thing(): void {} | ||
- | ||
+<no content> |
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.
[nitpick] The logic within tryLoadInputFileForPath is highly complex with deeply nested conditionals and loops (particularly lines 757–871). Consider extracting portions of this logic into smaller, well-named helper functions to improve readability and ease testing.
Copilot uses AI. Check for mistakes.
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.
That ship sailed long ago, pal
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 don’t think this complex logic needs to be ported at all—inferring the rootDir based on file inclusion is deprecated in 6.0, except in tsconfig-less compilations, which seems unlikely to happen with
outDir
/declarationDir
anyway. I objected to the guessing logic when it went into Strada but was overruled; it seems even more unnecessary now.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 I'm confused, because the reason I'm porting this is to make self-reference imports map properly, per the issue I'm fixing where if we don't do this we get output dts files in the program.
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.
Sorry, I wasn’t specific enough—this function needs to exist, but I think it should just error (which we won’t actually collect now, as you noted) and return a failed resolution if it can’t compute a
rootDir
instead of doing all the complicated guessing.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 see; I'll give that a shot. This PR was me just asking copilot to port the whole thing which worked, so...
#1304 is my followup to actually report these diags, which I could flip to point at main first.
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.
Maybe I'm missing something, but how can I actually emit this error? If I stop the resolution here by adding an error and then return
&resolved{}
, then all other later resolution steps stop working and hundreds of tests break.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.
If I just say to continue searching, then we end up with the same behavior again, where we end up with output files in the input set, and package import mappings stop working...