Skip to content

Composite projects: support strict dependencies #36743

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

Open
alexeagle opened this issue Feb 11, 2020 · 4 comments
Open

Composite projects: support strict dependencies #36743

alexeagle opened this issue Feb 11, 2020 · 4 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@alexeagle
Copy link
Contributor

Google (and Bazel rules_typescript) use a special-purpose TypeScript compiler. One of the reasons for this is we want to enforce strict dependencies.

Strict dependencies means you may only import or reference symbols declared in a project you explicitly declare to be an input. This is an essential property in a large monorepo as a defense against accidental coupling between projects.

For example we might have three directories, A B and C:

alexeagle@alexeagle:~/Projects/repro_strict_deps$ cat A/a.ts
export const a = 'hello';
alexeagle@alexeagle:~/Projects/repro_strict_deps$ cat A/tsconfig.json 
{
    "compilerOptions": {
        "composite": true
    }
}
alexeagle@alexeagle:~/Projects/repro_strict_deps$ cat B/b.ts
import {a} from '../A/a';

export function sayHello(f: string) {
    console.log(a + f);
}
alexeagle@alexeagle:~/Projects/repro_strict_deps$ cat B/tsconfig.json
{
    "compilerOptions": {
        "composite": true,
    },
    "references": [
        {"path": "../A"}
    ]
}
alexeagle@alexeagle:~/Projects/repro_strict_deps$ cat C/c.ts
import {a} from '../A/a'; // SHOULD FAIL HERE
import {sayHello} from '../B/b';

sayHello('world');
console.error(a);
alexeagle@alexeagle:~/Projects/repro_strict_deps$ cat C/tsconfig.json
{
    "references": [
        // NO REF TO A
        {"path": "../B"}
    ]
}

Let's say we work on library B. We decide we no longer need to depend on A, so we remove the dependency, but this breaks users such as C. This is a serious impediment to good code practices because our dependency on A leaked to users who reference its symbols without declaring their own dependency. In a big monorepo, the breakage of removing a dependency can prevent ever cleaning them up

We would like to propose a strictness setting when used with Project References, such that imports (or ambient references) from transitive dependencies should be disallowed.

/cc @evmar @josephperrott

@RyanCavanaugh
Copy link
Member

Possibly dumb question, isn't this just rootDir: "./" ?

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Feb 21, 2020
@alexeagle
Copy link
Contributor Author

I'm not sure how rootDir would help here. In project C I want TypeScript to allow the import from B but not from A. I tried adding rootDir: "./" in C/tsconfig.json and can still repro that it allows dependencies on A even though the project reference isn't declared.

@TimvdLippe
Copy link
Contributor

TimvdLippe commented Apr 23, 2020

I wanted to do something similar, and I also ran into this issue with project references in our Ninja-TypeScript integration for Chromium. My attempt was to use noResolve, which seemed to work initially, until I discovered that project references don't work with a different outDir. E.g. TSC will resolve to the original source files, but uses the .d.ts generated in the outDir:

front_end/common/common.js:

import * as Platform  '../platform/platform.js';

front_end/platform/platform.js:

export const foo = 42;

If you then have your tsconfig.json generated in gen/front_end/common/tsconfig.json which points its files to ../../../front_end/common/common.js and its project references to ../platform, it will use the gen/front_end/platform/platform.d.ts, but only after resolving to front_end/platform/platform.js. Then TSC gets confused, as it thinks that front_end/platform/platform.js is not part of ../platform (gen/front_end/platform/) even though it is listed in its files and complains that it can't find the import:

front_end/common/Color.js:30:27 - error TS7016: Could not find a declaration file for module '../platform/platform.js'. '/front_end/platform/platform.js' implicitly has an 'any' type.

30 import * as Platform from '../platform/platform.js';
                             ~~~~~~~~~~~~~~~~~~~~~~~~~

Note that the declaration file does exist in gen/front_end/platform/platform.d.ts, the platform.js is listed in the files and I have verified that if I make changes to platform.d.ts it does properly fail the compilation on gen/front_end/common/tsconfig.json (when using noResolve: false).

This seems very similar to the issue described in #27572

@TimvdLippe
Copy link
Contributor

We have several external contributors for Chrome DevTools and this error message is among the top of confusing error messages for our contributors. By missing a single dep in our build system, tsc can easily spit out hundreds of lines of errors containing various files that it detected but were missing. Enforcing strictness for these errors is also on my list to workaround in https://bugs.chromium.org/p/chromium/issues/detail?id=1211289, but having a solution in the compiler would probably be a lot more maintainable.

This feature is also available in other ecosystems such as Java in Bazel: https://blog.bazel.build/2017/06/28/sjd-unused_deps.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants