Provide contol over paths of inserted imports in ivy library build

[bgotink]

Which @angular/* package(s) are relevant/releated to the feature request?

compiler-cli

Description

Currently the ivy library compiler tries to guess where it has to find modules, directives, components, or pipes when it has to introduce new imports. As far as I can tell, it does this based on whether the target is an APF entry point or not.

This doesn't work well in our use case, which is a monorepository managed via yarn workspaces. In other words, local packages are installed in node_modules in the repo itself, and will be built into APF npm packages and deployed later on. We don't have paths in (most of) our tsconfigs, as node's resolution algorithm is perfectly capable of handling our setup. But, because the files aren't in APF locally, the compiler inserts deep imports, e.g. ../../node_modules/@scope/pkg/src/my.module rather than @scope/pkg.

This yields errors when using ng-packagr, as can be seen in this example repo: https://github.com/bgotink/angular-repro-20210826. The error is the same as the one seen in #38876.

We've got our own (closed source) library pipeline which "corrects" the paths the ivy compiler adds, to run the relative import back into a bare package specifier. This is done via a beforeTs transformer in Program#emit's customTransformers option. We cannot implement the same behaviour in the .d.ts files though, as there is no afterDeclaration transformer in the angular Program (typescript's Program has it though). This leads to packages being published with invalid types. I've included the output of our internal pipeline in the repository linked above. This transformer actually already existed before we switched to ivy. It was introduced to work around #23917. We extended it when we switched to ivy, without considering the root cause of the new import paths.

Proposed solution

Provide more control over inserted imports in the APIs of @angular/compiler-cli. While the compiler-cli itself doesn't know the entire context it is used in, and it has to make certain assumptions, the consumer of the compiler-cli package has more understanding of the context and can make more educated decisions.

In our scenario we would want the compiler to always use bare package specifiers for imports that are not in the entry point currently being built.

Alternatives considered

• Consider non-APF packages inside a node_modules folder as "these will be APF when published" rather than "this is a local private package". Whether this is a viable option is questionable at best, as there might be repositories where local private packages are linked via yarn workspaces This could be turned on/off via an option in the compiler-cli APIs or via angularCompilerOptions.
• Provide an afterDeclaration custom transformer option in Program#emit. This would allow us to continue hacking around the inserted imports by replacing relative imports into node_modules with bare package specifiers.

Alternatively, do nothing in angular. We could build our entire monorepository in topographical order (dependencies before dependants) rather than the order in which the projects occur in angular.json, which would make it possible to use tsconfig paths to load the built APF packages for local dependencies. This would work, but…

• It would require all (transitive) dependencies always be built to build any single package. This conflicts with our test pipeline which, like nx's affected feature, builds only the packages impacted by the PR to dramatically speed up the pipeline for large projects.
• It would lead to confusing behaviour if someone forgets to rebuild a dependency, as the tsconfig would lead to the previous version being used. This is especially true as we still want to resolve dependencies to the typescript source in our IDE. Added to that the number of mistakes with our generated @microsoft/api-extractor API reports would skyrocket.

[petebacondarwin]

Is this problem related to, or solved by the generateDeepReexports compiler option? https://github.com/angular/angular/blame/master/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts#L264-L293

[bgotink]

@petebacondarwin That option doesn't seem to make any difference. In the example repo in the OP ng-packagr still yields the same error if I turn generateDeepExports on or off, and our internal pipeline still yields the same output.

The documentation of generateDeepExports also makes me think it isn't the right fit: the local dependency that's being import is not in the APF when the packages are built, but it will be APF when installed as dependency in downstream projects.

[JoostK]

I looked at the repo a bit and I think that the root cause here is that OneModule is consumed from source, which exposes the implementation of OneModule.forRoot to Angular's static interpreter (instead of exposing just the .d.ts file). When interpreting function calls based on .d.ts metadata the compiler uses a "foreign function resolver" to extract an expression that it can evaluate:

https://github.com/angular/angular/blob/c721135e370b34c840756bcfb22c8119b4c8c452/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts#L457-L477

Importantly, it makes an effort to switch the evaluation context to start tracking the originating import specifier if the function was imported using an absolute module specifier.

For functions with body, the compiler instead uses the current evaluation context regardless of how the function is imported:

https://github.com/angular/angular/blob/c721135e370b34c840756bcfb22c8119b4c8c452/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts#L480-L497

When applying the same technique as for foreign functions, as follows, your example compiles correctly using @scope/one as module specifier. I added the following logic just before evaluating the function body:

    // If the function occurs in a different file, then assume that the owning module
    // of the call expression should also be used for evaluating the function body.
    if (fn.node.getSourceFile() !== node.expression.getSourceFile() && lhs.bestGuessOwningModule !== null) {
      context = {
        ...context,
        absoluteModuleName: lhs.bestGuessOwningModule.specifier,
        resolutionContext: lhs.bestGuessOwningModule.resolutionContext,
      };
    }

I believe this is the correct thing to do, as the compiler generally assumes that an absolute import path indicates a different entry-point.

[JoostK]

@alxhub does that sound like a reasonable change to you?

[bgotink]

We've been consistently running into this issue once every few months, and for now our solution has been to replace the forRoot() function with an alternative set-up (e.g. a second root-only module). We are now starting to run into cases where this becomes harder to do.

@JoostK Is it feasible to land the fix you proposed or to add the necessary hooks for us to work around the problem?

EDIT: Right after writing this comment I realised that the emitCallback does give us access to the afterDeclarations hook on the typescript Program, so I have been able to update the same workaround we already had for the .js files to also work for .d.ts files.