Typescript: Allow configuration of NodeBuilderFlags.AllowNodeModulesRelativePaths for un-distributed mono repos

[joeljeske]

Search Terms

• AllowNodeModulesRelativePaths
• The inferred type of ... cannot be named without a reference to ...
• #29808
• https://github.com/microsoft/TypeScript/issues/30858
• https://github.com/microsoft/TypeScript/pull/27340

Suggestion

Allow the optional use of AllowNodeModulesRelativePaths during declaration creation

Use Cases

I understand why @weswigham added https://github.com/microsoft/TypeScript/pull/27340 and it is correct for most consumers, especially those that publish packages for consumption on NPM.

However, it seems like for npm style mono-repos where packages are only consumed locally and not published to a registry, it is valid to have seemingly non-portable TS references (lerna, pnpm, rush, yarn workspaces). A relative path to a node_module may still be valid as long as the structure stays consistent, which it does for this style of mono repo with a single node_modules directory.

I am using a mono repo managed by Bazel and rules_nodejs and users are experiencing issues with the creation of declaration files that otherwise would contain relative paths to node_module files. https://github.com/bazelbuild/rules_nodejs/issues/1013

(The conversation in https://github.com/microsoft/TypeScript/issues/30858 was helpful, however, it seemed to still be focusing on resolving portable types, which is only necessary when distributing the declaration files. If they are consumed locally, portability is not a concern.

Examples

I'm not sure the best way (or standard way) for these flags to be set, however, I imagine an entry in the TSConfig Compiler Options would make the most sense:

interface CompilerOptions {
  ...
+  allowUnportableDeclarationImports: true
} 

Currently I am patching Typescript to toggle on this behavior and it appears it is working just as expected.

typescript+3.7.3.patch.txt

Checklist

My suggestion meets these guidelines:

• [X] This wouldn't be a breaking change in existing TypeScript/JavaScript code
• [X] This wouldn't change the runtime behavior of existing JavaScript code
• [X] This could be implemented without emitting different JS based on the types of the expressions
• [X] This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
• [X] This feature would agree with the rest of TypeScript's Design Goals.

[RyanCavanaugh]

@weswigham I'm interested to hear your thoughts on this

[weswigham]

@joeljeske you only want this for incremental builds, right? Since if you trigger this error there's no way you're shipping the resulting .d.ts file as a redistributable.

[joeljeske]

Well no actually. I am using bazel with rules nodejs, but let me describe how it works. The build consists of many packages that link to each other via tsconfig path mappings (not via symlinking into node modules). Each package is built independently and only sees the type declarations of other local dependencies. The local dependencies all share a common top level node modules directory. In this way, if a package generates a declaration file with a relative import path into the node modules dir then downstream local packages that depend on that declaration file would still be able to resolve that import.

None of the declarations files are ever used outside the mono repo

Does that make much sense?

[weswigham]

Sure; but what are the declaration files being built for? It's certainly not for publishing for consumption if you're OK with paths like this (since there's absolutely no way these paths could ever be shipped outside a specialized environment), so the only other use would be incremental style (similar to --incremental or --build mode) builds. As far as I know, bazel's default mode of operation, too, is incremental in nature.

[joeljeske]

The declaration files are used for type checking against other local packages. They never leave the repo.

Example: local module A depends on local module B. The generated declaration files of B are used during the compilation of A.

Yes bazel is incremental, but not in the sense of using —incremental as far as I know. It is incremental in the sense that module A will only be recompiled if the declaration files (public api) of module B changes.

My mono repo has many small Packages in typescript that are used in a variety of places. The end product of the mono repo is a set of compiled JS applications ready for runtime. the d.ts files are only used in the intermediary build process as described.

[weswigham]

The declaration files are used for type checking against other local packages. They never leave the repo.

Right - pretty much the definition of an incremental build. I can see removing the error when incremental is set but declaration is not expressly set, however directly exposing an internal like this is not something I'd be comfortable with.

[joeljeske]

Yea I guess i was thinking about an incremental build from the standpoint of a single package. In my case each package is fully rebuilt at a time, not using the incremental TS option. But looking from the standpoint of the entire repo, yes it is incremental; some packages will have TSC invoked on them and some will not.

Regardless of the terminology, I can say that I am not using TS incremental option and I am explicitly requesting the declaration files. I do respect concern of exposing implementation details, however, I feel like I have a valid use case here that does not fit into your limited exception:

when incremental is set but declaration is not expressly set

[flolu]

I definitely agree, that @joeljeske has a valid use case here.

Especially, I do not think it would hurt anybody if it is disabled by default and can be enabled in the compiler options when needed.

[joeljeske]

@weswigham @RyanCavanaugh, do y'all have any additional thoughts regarding my use case? Would any supporting examples or additional use case explanations be of help?

[matthewjh]

@weswigham we would really appreciate your help in prioritising a proper solution for this. Yes, the d.ts files are intermediate artefacts as part of a larger incremental build.

FWIW we are trying to move our company-wide monorepo to bazel, and though we currently have a workaround in a patch provided by @flolu and @joeljeske, this is not really production-appropriate and we strongly feel this needs to be properly supported by vanilla tsc.

Note that:

1. This will affect anyone who uses bazel and the ts_library rule, AND who uses type inference.
2. This is fully reproducible in the official Angular repo, which uses bazel and ts_library. By checking out and changing the code so that tsc has to add an import for an inferred type, you will encounter the error. They seem to have either gotten lucky or deliberately worked around by avoiding any need for type inference, which I find very surprising. So - this is not a weird edge case-type issue, but affects existing consumers of tsc and bazel in large, popular codebases.

Thank you, and we appreciate the help.

[btakita]

It looks like this feature has been delayed. Are there any good workarounds other than sucking it up & dealing with thousands of failures individually by casting each failure with any type or explicitly typing each failure?

@RyanCavanaugh @weswigham I would prefer to expose internal types as I utilize both a set of monorepo open source libraries used by multiple monorepo private apps. Please take this issue seriously. It's cost me > 10 hours of work so far in the middle of a large refactoring under pressure from a client.