microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.31k stars 12.53k forks source link

Declaration emit reveals paths within dependency that were not referred to in the source file #38111

Open mheiber opened 4 years ago

mheiber commented 4 years ago

Context:

Declaration emit reveals paths within dependency that were not referred to in the source file:

export declare const instance: import("dependency/internal-types").TheInterface;

This is a problem because changing paths within a dependency can break a dependent package.

TypeScript Version: 3.9.0-dev.20200212

Search Terms:

Code

node_modules/@types/dependency/entrypoint.d.ts

import { TheInterface } from "./internal-types";
export { TheInterface };
export declare function getInstance(): TheInterface;

node_modules/@types/dependency/internal-types.d.ts

export interface TheInterface {}

node_modules/@types/dependency/index.d.ts

// empty - doesn't matter for this example

example.ts

import { getInstance } from "dependency/entrypoint";
export const instance = getInstance();

src/app/tsconfig.json

{
    "compilerOptions": {
        "declaration": true,
    }
}

Expected behavior:

example.d.ts

export declare const instance: import("dependency/entrypoint").TheInterface;

Actual behavior:

example.d.ts

export declare const instance: import("dependency/internal-types").TheInterface;

It seems that part of the solution could involve having the compiler avoid using relative paths in types (import("<relativepath>").<typename>) if the relativepath is outside the project.

Repro Repo: https://github.com/mheiber/repro-rel-import-inlining

Related Issues:

Issue written with the help of: @rricard, @robpalme and @mkubilayk

rricard commented 4 years ago

When writing up that issue we also found out a pretty interesting effect.

If you were to move the internal-types.d.ts into a sub-directory in the dependency and have entrypoint load it:

node_modules/@types/dependency/entrypoint.d.ts

import { TheInterface } from "./int/internal-types";
export { TheInterface };
export declare function getInstance(): TheInterface;

We do get the wanted declaration output:

example.d.ts

export declare const instance: import("dependency/entrypoint").TheInterface;

This is great because that means that TypeScript has a notion of some files are meant to be imported and others do not.

At the moment it seems like everything in the root of the dependency is considered importable externally while subdirectories are not. Actually the rule seems to be, whoever is the least deep gets it:

checker.ts:5014

                    function sortByBestName(a: number, b: number) {
                        const specifierA = parentSpecifiers[a];
                        const specifierB = parentSpecifiers[b];
                        if (specifierA && specifierB) {
                            const isBRelative = pathIsRelative(specifierB);
                            if (pathIsRelative(specifierA) === isBRelative) {
                                // Both relative or both non-relative, sort by number of parts
                                return moduleSpecifiers.countPathComponents(specifierA) - moduleSpecifiers.countPathComponents(specifierB);
                            }
                            if (isBRelative) {
                                // A is non-relative, B is relative: prefer A
                                return -1;
                            }
                            // A is relative, B is non-relative: prefer B
                            return 1;
                        }
                        return 0;
                    }

27340 / 7a71887c23a110009bc974f626245f03066e6926 by @weswigham

Is that behavior intentional, in which case we can use it as a workaround, or is this arbitrary behavior that can change?

rricard commented 4 years ago

Additionally the example here leverages node_modules but we do not use it in our actual system. Instead we use compilerOptions.paths, the same issue and the same rootDir/subdir effect is observable with paths. If you are interested in an example with paths I can also provide one.

weswigham commented 4 years ago

Preferring he shortest (in path segments) absolute path we find is intended behavior, yes.

mheiber commented 4 years ago

@weswigham thanks for getting back to us. Is the "prefer the shortest (in path segments) path" behavior that is likely to not change in future releases?

If so, we can use it to hack around the problem we're seeing:

Ideally, there would be a non-hacky userspace solution, but the subdirectorification trick could work in the short term.

weswigham commented 4 years ago

It is the only heuristic we currently use to sort all found import paths. The only other heuristic I could see us maybe using in the future is combining it with a "longest alias chain to source" (minus cycles) to break ties, but calculating that would be crazy expensive compared to just this (which is essentially a string comparison and loosely tracks the same thing in conventional project structures). Since we have no interest in making the already sluggish declaration emit slower, we're unlikely to change it anytime soon, I think. And even then, we'd still be using segment count as the initial sort and filter, so if you were relying on it, you'd be fine.

robpalme commented 4 years ago

@weswigham One additional heuristic that might help solve this problem in future would be to favour explicit package entrypoints as designated in pkg.json "exports".

That would ensure generated declaration files use the same encapsulation boundaries defined by the source code's modules & packages.

weswigham commented 4 years ago

When we add support for the new esm and cjs resolvers in node, yeah, probably.

robpalme commented 4 years ago

For the record, we solved this problem by making our build tool inject ambient module declarations for each external dependency into a generated file we call ambient.d.ts.

// ambient.d.ts
declare module "my-dependency" {
    export * from "../../../path/to/dependency";
}

Ambient module declarations and tsconfig "paths" make look like two ways of solving the same problem (wiring up a bare-specifier to a known location on disk), but they are not equivalent! Ambient module declarations have an additional super-power: they register the bare-specifier as a first-class dependency in the TypeScript resolver. This causes declaration emit to prefer (see sortByBestName) outputting the bare-specifier over the relative path.

Maybe we should update the documentation for "paths" to make it they are not the best way to refer to dependencies?

eps1lon commented 3 years ago

For the record, we solved this problem by making our build tool inject ambient module declarations for each external dependency into a generated file we call ambient.d.ts.

@robpalme Could you setup a complete example? When I try it with

declare module '@material-ui/core' {
  export * from './packages/material-ui/src/';
}

I get Import or export declaration in an ambient module declaration cannot reference module through relative module name..

How would this work for wildcard paths like "@material-ui/core/*": ["./packages/material-ui/src/*"],?

remcohaszing commented 2 years ago

This has gotten worse with the introduction of "moduleResolution": "node16". Paths are now rewritten to relative paths inside node_modules. (import('mdast-util-from-markdown') yields import("../../../node_modules/mdast-util-from-markdown/lib/index.js"))

https://github.com/remarkjs/remark/issues/1039#issuecomment-1239253894