Build better import paths for declaration emit/typeToString from reexports if possible #27340
Conversation
…node_modules import generation
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 4f50766. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@Andy-MS you wanna look over this? |
|
@rbuckton done. |
src/compiler/diagnosticMessages.json
Outdated
| @@ -2513,6 +2513,10 @@ | |||
| "category": "Message", | |||
| "code": 2738 | |||
| }, | |||
| "The inferred type of '{0}' requires a reference to a module at '{1}' to name, which would likely break on type redistribution. A type annotation is necessary.": { | |||
There was a problem hiding this comment.
is {1} the name of the package? Or the module itself?
There was a problem hiding this comment.
What about
Generating a declaration for '{0}' requires generating a reference to the module '{1}'; however, this is susceptible to breaking upon distribution. Provide an explicit annotation or add an explicit dependency on '{2}'.
There was a problem hiding this comment.
From the new test:
The inferred type of 'x' requires a reference to a module at 'foo/node_modules/nested' to name, which would likely break on type redistribution. A type annotation is necessary.
It's the specifier we would generate were we to emit it.
There was a problem hiding this comment.
We have an accessibility error
"The inferred type of '{0}' references an inaccessible '{1}' type. A type annotation is necessary."
which this message is templated after. I imagine we should use consistent language.
| if (links.extendedContainersByFile && links.extendedContainersByFile.has(id)) { | ||
| return links.extendedContainersByFile.get(id)!; | ||
| let results: Symbol[] | undefined; | ||
| if (links.extendedContainersByFile && (results = links.extendedContainersByFile.get(id))) { |
There was a problem hiding this comment.
This nesting is unnecessarily complicated. What about this:
let results = links.extendedContainersByFile && links.extendedContainersByFile.get(id);
if (results) return results;
src/compiler/diagnosticMessages.json
Outdated
| @@ -2525,6 +2525,10 @@ | |||
| "category": "Error", | |||
| "code": 2741 | |||
| }, | |||
| "The inferred type of '{0}' requires a reference to a module at '{1}' to name, which would likely break on type redistribution. A type annotation is necessary.": { | |||
There was a problem hiding this comment.
Naming the type of '{0}' requires a reference to a module at '{1}' to name, which would likely break on type redistribution. A type annotation is necessary.
or
``
Generating a declaration for the type of '{0}' requires a reference to a module at '{1}' to name, which would likely break on type redistribution. A type annotation is necessary.
There was a problem hiding this comment.
The inferred type of '{0}' cannot be named without a reference to '{1}'. This is likely not portable. A type annotation is necessary. ?
weswigham
force-pushed
the
lookup-reexports-for-better-imports
branch
from
b966829 to
278149c
Compare
Also, issue an error on relative/deep
node_modulesimport generation (undermoduleResolution: "node") if a better specifier cannot be found (which can happen if your type definitions need to depend on the types of a dependency of a dependency which you do not yet directly depend on). While such paths technically work on the compiling system, the resulting.d.tsfile will be non-portable; this can cause issues for package consumers, so it is best to mark it as needing a type annotation to fix the problem.Fixes #26985