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 ...
• Package dependencies and The inferred type of ... cannot be named without a reference to ... #29808
• types in transitive dependencies causing errors after upgrade from 3.1: The inferred type of ... cannot be named without a reference to #30858
• Build better import paths for declaration emit/typeToString from reexports if possible #27340

Suggestion

Allow the optional use of AllowNodeModulesRelativePaths during declaration creation

Use Cases

I understand why @weswigham added #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. bazel-contrib/rules_nodejs#1013

(The conversation in #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:

• This wouldn't be a breaking change in existing TypeScript/JavaScript code
• This wouldn't change the runtime behavior of existing JavaScript code
• This could be implemented without emitting different JS based on the types of the expressions
• This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
• 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.

[sethfowler]

I hope this can be addressed soon. Like the other commenters above, this proved so problematic when trying to transition our build system to Bazel that we had to resort to patching the compiler.

@weswigham, I'm sure you have a lot on your plate and I know the challenge of juggling competing priorities, but if there's any possibility of getting an option that would disable this error into an upcoming TypeScript release, it would be super helpful to Bazel users.

[weichensw]

My use case is different than @joeljeske that I am not making a bazel monorepo but a monorepo that is based on project references.

I have a project P1 that exports a type T1 that derived from P1's node_modules type T2. P2 is another project that references P1 in the same monorepo using tsconfig's references. When P2 uses T1, I got:

The inferred type of cannot be named without a reference to 'P1/node_modules/<some_package>/lib' this is likely not portable.

@andrewbranch looked into this case and pointed out that if I re-export T2 from P1 then the error will go away, I will be OK for the time being to re-export T2 but would rather not to if not required.

I think there's couple of things could be improved here:

• The error message is a bit misleading in that it seems to demand a direct reference from P2 to <some_package> but in-fact only need a re-export from P1 of type T2
• In a referenced project, we don't really need to check for portability because the referenced project is not going to be shipped as a npm package. The d.ts files are just for incremental compilations purpose.

Personally, I would be more than happy to inform TS in my tsconfig that I am not going to ship current project as a npm package in order for TS to safely skip some checks in exchange of flexibility or performance. Or if TS infer that automatically from composite: true it would work for my case too and makes perfect senses for me.

[weswigham]

Honestly, the ecosystem issues having a flag that allows this opens up aren't worth the hassle. A simple flag that allows this would be misused (guaranteed) - the correct fix, in every circumstance, is just to do as the error asks and explicitly type annotate the position the error is on. That both speeds up the compiler and keeps the declarations predictably portable.