Skip to content
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

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

Open
mheiber opened this issue Apr 22, 2020 · 10 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@mheiber
Copy link
Contributor

mheiber commented Apr 22, 2020

Context:

  • Generating declaration files

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

@mheiber mheiber changed the title Declaration emit includes relative inline imports within its dependencies Declaration emit leaks relative paths from dependencies Apr 22, 2020
@mheiber mheiber closed this as completed Apr 22, 2020
@mheiber mheiber changed the title Declaration emit leaks relative paths from dependencies Declaration emit leaks directory structure from dependencies Apr 22, 2020
@mheiber mheiber changed the title Declaration emit leaks directory structure from dependencies Declaration emit reveals paths within dependency that were not referred to in the source file Apr 22, 2020
@mheiber mheiber reopened this Apr 22, 2020
@rricard
Copy link

rricard commented Apr 22, 2020

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 / 7a71887 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
Copy link

rricard commented Apr 22, 2020

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
Copy link
Member

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

@mheiber
Copy link
Contributor Author

mheiber commented Apr 23, 2020

@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:

  • we can move all non-entrypoints of our package into a subdirectory

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

@weswigham
Copy link
Member

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
Copy link

@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
Copy link
Member

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

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jun 1, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.0 milestone Jun 1, 2020
@robpalme
Copy link

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
Copy link
Contributor

eps1lon commented Dec 31, 2020

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
Copy link

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"))

remarkjs/remark#1039 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

No branches or pull requests

10 participants