Repository navigation
Allow configuration of NodeBuilderFlags.AllowNodeModulesRelativePaths for un-distributed mono repos #37960
Description
Activity
Wesley Wigham (@weswigham) I'm interested to hear your thoughts on this
Joel Jeske (@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.
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?
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.
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.
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
incrementalis set butdeclarationis not expressly set, however directly exposing an internal like this is not something I'd be comfortable with.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
Reacted by Florian- added a commit that references this issue
on Apr 28, 2020 I definitely agree, that Joel Jeske (@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.
Reacted by Scott BlumWesley Wigham (@weswigham) Ryan Cavanaugh (@RyanCavanaugh), do y'all have any additional thoughts regarding my use case? Would any supporting examples or additional use case explanations be of help?
15 remaining items
My use case is different than Joel Jeske (@joeljeske) that I am not making a bazel monorepo but a monorepo that is based on project references.
I have a project
P1that exports a typeT1that derived fromP1's node_modules typeT2.P2is another project that referencesP1in the same monorepo using tsconfig'sreferences. WhenP2usesT1, I got:The inferred type of cannot be named without a reference to 'P1/node_modules/<some_package>/lib' this is likely not portable.
Andrew Branch (@andrewbranch) looked into this case and pointed out that if I re-export
T2fromP1then the error will go away, I will be OK for the time being to re-exportT2but 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
P2to <some_package> but in-fact only need a re-export fromP1of typeT2 - 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.tsfiles 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: trueit would work for my case too and makes perfect senses for me.Reacted by Karol Kozicki, Tomas Reimers and Vishnu- The error message is a bit misleading in that it seems to demand a direct reference from
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.
Search Terms
Suggestion
Allow the optional use of
AllowNodeModulesRelativePathsduring declaration creationUse Cases
I understand why Wesley Wigham (@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: