Check nearest package.json dependencies for possible package names for specifier candidates #58176
Conversation
…r specifier candidates
typescript-bot
added
Author: Team
For Uncommitted Bug
src/compiler/moduleSpecifiers.ts
Outdated
| if (resolvedPath === moduleFileName) { | ||
| return name; | ||
| } | ||
| if (host.realpath && host.realpath(resolvedPath) === host.realpath(moduleFileName)) { |
There was a problem hiding this comment.
I didn't want to have to realpath here, but it seems to be the only way we can check if the two paths actually refer to the same thing through symlinks.
|
I've been reading the comments on #42873 and I'd estimate anywhere from 30% to 80% are legitimate misconfigurations, or at least configurations that we can't plausibly disambiguate from incorrect ones. It's hard to tell. This looks like a good fix but I'll also write up a FAQ entry on this which covers the basics. What's the most-plausible way someone would legitimately hit this error in a way that is also "fixable" without some restructuring of their package folder layout? |
The test case in this PR is a good example - a workspace ( |
draft faqLet's say you use a package manager with strict dependencies: Where import { getMakeSubDep } from "other_package";
// The inferred type of p refers to a type defined
// inside node_modules/other_package/node_modules/sub_dep/index.d.ts
export const p = getMakeSubDep();When TypeScript needs to emit export const p: ???What should go in the One option would be to use a relative path: import { subdep } from "../node_modules/other_package/node_modules/sub_dep";
export const p: subdepThis is obviously wrong: It's implausible that a consumer of Another option would be to use a subpath: import { subdep } from "other_package/node_modules/sub_dep";
export const p: subdepThis is also obviously wrong: The semantics of Another option would be to use a module name: import { subdep } from "sub_dep";
export const p: subdepIs this correct? If If you don't have a declared dependency on These situations - the non-working ones - are the "non-portable identifiers" that TypeScript is complaining about. TS was put into a position where it had to reference the name of a module, but it couldn't find a name for that module that appeared to work. This is why this error occurs. Now, if you do have a declared dependency on But! If you never referred to The correct way to address this is generally to import the type that the // in foo.ts
import { subdep } from "sub_dep";
import { getMakeSubDep } from "other_package";
// No additional type annotation needed
export const p = getMakeSubDep();If you can't do this, then TypeScript can't either, and the only real alternative is to use a broader type annotation so that TS doesn't need to refer to types which are impossible to name: // in foo.ts
import { getMakeSubDep } from "other_package";
// Annotate to anonymous version of whatever subdep is
export const p: { isSubDep: boolean } = getMakeSubDep();Or restructure the input file in such a way that the referenced type is not exposed in the |
|
Yep, excellent summary of the current state of affairs. That's basically all the productive bits of the years of discussion in #42873 summed up. |
|
For the FAQ, another option to offer up is to extract the type from the original import to avoid adding additional (fragile?) routes to the type. export const p: ReturnType<getMakeSubDep> = getMakeSubDep();(Even better would be if this could be auto-inferred during DTS emit to avoid the user needing to explicitly perform the extraction in the TS file.) |
|
Is there an option to enforce that whatever
@RyanCavanaugh I don't see how this is wrong.
@robpalme |
The point of this is that |
|
Right, I misread the package name. What about
then? |
I don't understand the proposal. If the package fully conceals its dependencies, as it should, then you won't have a problem. If it doesn't, you may or may not have a problem depending on how things go. If you turn on this rule, you'll always have a problem. Having more problems doesn't seem like a solution? |
|
I use an automated script as a workaround, help TypeScript find the type for non-flattened dependency directory structures ( like pnpm ), first locate the real position of the indirect dependencies: const otherDepPath = path.dirname(require.resolve('other_package/package.json'))
const subDepPath = path.dirname(
require.resolve(`sub_dep/package.json`, { paths: [otherDepPath] })
)then add /// <reference types="${subDepPath}" />if the // tsconfig.json
{
"compilerOptions": {
"paths": {
"sub_dep": ["/Users/path/to/pkg/dir"] // `${subDepPath}`
}
}
} |
Hm, maybe not too bad? If we use a not-direct-match result that has the same affecting |
…andles deep imports
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
@typescript-bot test it @andrewbranch slightly new approach here - now, rather than looking for a |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
…crese the lliklihood of caching a relevant symlink)
|
@typescript-bot test it |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the user tests comparing Everything looks good! |
|
@sheetalkamat extended tests are still good, and I think I've addressed all your concerns. |
|
@weswigham Here are the results of running the top 400 repos comparing Everything looks good! |
| for (const field of runtimeDependencyFields) { | ||
| const deps = packageJson[field]; | ||
| if (deps && typeof deps === "object") { | ||
| result = concatenate(result, getOwnKeys(deps)); |
There was a problem hiding this comment.
You can probably just use append here, given getOwnKeys is going to give a fresh copy anyway.
There was a problem hiding this comment.
Eh, wrong argument type. Nevermind.
There was a problem hiding this comment.
You'd want addRange, not append, since append doesn't handle an array for input or take a spread list of arguments. But even that's not perfect - concatenate unconditionally allocates a new output array if both inputs are present (which is actually going to be rare here, usually it'll just be dependencies) - meanwhile addRange unconditionally copies from if to is missing (so it guarantees returning either to to a new array). That means it'd copy the first array needlessly every time. We actually don't have a "use either array mutably if they're present, it's fine"-type array-merging helper.
Honestly, it probably doesn't matter, though. Resizing one temporary array to merge in another one is probably going to trigger the similar allocations in the VM that making a new array would anyway (if they're both reasonably large). It's all short-lived objects.
Implements this comment.
I won't say this "fixes" #42873, but it does prevent it from occurring for people in workspaces with appropriately set-up explicit dependencies in their package files, even when they haven't explicitly referenced all of those dependencies in their program. IMO, that's the upper limit of how much we can or should fix that issue - anything further would imply unsafe/unportable specifiers, which I don't want us to generate. I say that, but this could be improved upon: While this leverages the fact that we already lookup all
package.jsonfiles to make this cheap, this does not look through thosepackage.jsondependenciesto try and make a deep import, so we'll only discover top-level package imports this way right now. That's a potential improvement that could be built on this.