bug(migrations): migration-v13 unresolved TS project for non-buildable / test target without tsconfig path projects

[Coly010]

Is this a regression?

• Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

No response

Description

When running the migration (migration-v13), the following is printed to the console:
Could not find TypeScript project for project: project-name

This appears to be the result of the logic that is being used to find the TSConfig file for the project in the angular.json.
It is looking for test targets that use the @angular-devkit/build-angular:karma builder, ignoring that there might be other builders being used for the test target that contain a path to the tsconfig file.

After more investigation, I can narrow this down a bit more.
it's related to the getTargetTsconfigPath function.

Some library projects may not have a build target by default, and their test target may be using jest which does not contain a tsconfig property.

This definitely makes it more difficult to determine a tsconfig path. Especially as there may not be a standard convention for the lib's tsconfig file name.

Reproduction

Steps to reproduce:
1. Change the test target's builder of any project using Angular Material
2. Try run the migration

1. Have a project in angular.json that does not have a build target, or a tsconfig property in the test target

Expected Behavior

It should run the migration with no errors / warnings.

Actual Behavior

It does not find the tsconfig for the project therefore the migration logs warnings.

Environment

• Angular: 13.0.0
• CDK/Material: 13.0.0
• Operating System (e.g. Windows, macOS, Ubuntu): macOS

[devversion]

Thanks for the issue! With custom builders it is not safe/trivial to determine a path to an actual target tsconfig that works, and the migration would then fail down the road (causing even more confusion). I believe that is also not something we want to make any guarantees for.

As it seems we currently do not even check for an official builder (we do in ng-add), so I'm wondering what your target configuration for test looks like? please share this if you can make it public, thanks!

At first glance, I think there are two options:

• We keep checking for the tsConfig option without actually checking if this is an official CLI builder (like we do right now)
• Or we check for official builders and report an error to avoid confusion with the e.g. TypeScript project could not be found error.

[Coly010]

@devversion I've updated my original issue with corrected information as my initial thoughts proved to be incorrect.

After more investigation, I can narrow this down a bit more.
it's related to the getTargetTsconfigPath function.

Some library projects may not have a build target by default, and their test target may be using jest which does not contain a tsconfig property.

This definitely makes it more difficult to determine a tsconfig path. Especially as there may not be a standard convention for the lib's tsconfig file name.

As you've stated, you already do not look for an official builder, that was a mistake on my part.

Perhaps you can confirm this sentiment:
If an app consumes/depends on a library project in the workspace, will the migration run on the files within the library project?

• If that is true, is it possible to then ignore this library project from the migration, preventing the logger from warning about the unfound TS project.

[devversion]

@Coly010 I have discussed this a little more with the CLI team and it looks like we could change this to a warning where we just point to the changelog. Would that work for you?

I'm a little confused about:

preventing the logger from warning about the unfound TS project.

Is the update process still completing gracefully right now with just a warning? or does it stop migration other projects/targets in the workspace? I think the expected thing (at least for me), would be to continue migrating whatever project/target is possible, but just printing a yellow/red warning for the targets that could not be migrated. thoughts?

[Coly010]

@devversion
Unless I'm mistaken, the migration does the following:
1 . gets all projects in angular.json
2. looks at the build and test target to try determine the tsconfig path
3. if it does, it creates the TS Program and performs the migration
4. if it does not, it logs a warning saying it can't find the TS Project

Say I have a workspace of structure:

• apps/app1
• projects/lib1

lib1 does not have valid build or test target required to find the tsconfig therefore a warning is produced.

However, let's assume that app1 uses lib1. (e.g. import { something } from 'projects/lib1';)

The migration will successfully find app1's tsconfig and create the TS Program.
Does this mean that lib1 will still benefit from the migration, as it has been found via app1's TS Program analysing source?

If it does benefit from the migration because it has been used in app1, I'd think it would be desirable to not log that the migration couldn't find a tsconfig for lib1, because the migration still did run. This would cause confusion for the developers.

[devversion]

@Coly010 yeah, you are right. It looks like we just print a warning already.

The migration will successfully find app1's tsconfig and create the TS Program.
Does this mean that lib1 will still benefit from the migration, as it has been found via app1's TS Program analysing source?

That's a very interesting case. I think the answer is "partially". It depends on what files are referenced through/how the library tsconfig is composed. I imagine a scenario where only certain files are part of the TS resolution (when it visits an import), but some other files remain unvisited but would be technically part of another e.g. entry-point.

I think we should ensure that the migration never actually deals with files which are outside of the current project. We do this currently by relying on TypeScript to determine external files, but AFAICT it only skips source files in node_modules/**. So we need to update this IMO.

If it does benefit from the migration because it has been used in app1, I'd think it would be desirable to not log that the migration couldn't find a tsconfig for lib1, because the migration still did run. This would cause confusion for the developers.

Agreed, that is confusing. I think we should never migrate the lib1 (as it is external to the app1 project). Then we can keep the warning for the lib1 builder/project in a reasonable way.

[Coly010]

@devversion I agree that it would probably be better to skip visiting lib1 from the app1 TS Program.

However, in that scenario, is it, or can it be made possible, to allow the migration to be run manually with a flag such as --tsConfigPath="" where we can point to any skipped projects' tsconfig.

e.g. in the scenario outlined, app1 would be migrated but lib1 would not.
Therefore to migrate lib1 we could then run ng migrate lib1 --tsConfigPath="projects/lib1/tsconfig.json"

I've written a migration script using Nx Devkit which aims to perform the same migration. As you can see, it doesn't require the TSConfig at all, but that leaves me to ask, have I missed something in my thought process here? Is there a reason there is a TS Program being created?

export default async function (tree: Tree, schema: { projects: string[] }) {
  for (const project of schema.projects) {
    const projectConfig = readProjectConfiguration(tree, project);
    if (!projectConfig.sourceRoot) {
      continue;
    }

    visitNotIgnoredFiles(tree, projectConfig.sourceRoot, (filePath) => {
      if (filePath.endsWith('.scss') || filePath.endsWith('.css')) {
        // run migration
        const content = tree.read(filePath, 'utf-8');
        if (!content) {
          return;
        }

        const migratedContent = content.replace(
          /@(?:import|use) +['"]~@angular\/.*['"].*;?/g,
          (match) => {
            const index = match.indexOf('~@angular');
            return match.slice(0, index) + match.slice(index + 1);
          }
        );

        if (migratedContent && migratedContent !== content) {
          tree.write(filePath, migratedContent);
        }
      }
    });
  }

  await formatFiles(tree);
}

[devversion]

I've written a migration script using Nx Devkit which aims to perform the same migration. As you can see, it doesn't require the TSConfig at all, but that leaves me to ask, have I missed something in my thought process here? Is there a reason there is a TS Program being created?

It's true that this migration could technically run without a TS program here, but the migration has just leveraged the update tool we built for all types of migrations, and I think for the sake of a consistent way to have migrations - this is not too bad.

The update tool relies on the TypeScript program as it analyzes the sources and also determines inline templates, inline stylesheets etc. so it's actually doing much more than just checking for .scss and .css files in the devkit tree (while it also allows for convenient TS AST transformations + the migrations could technically run inside google as well).

Obviously the specific migration you mentioned does not even migrate inline templates, but you get the idea; and this issue does not seem to be about this specific migration as the TS program issue will appear with any future ng update as well, right?

[Coly010]

@devversion yeah exactly, the real issue here is that the TS Program will have similar future issues during migration 🙂

[devversion]

yeah, I think we need some more thoughts on this. I think a first step is to stop migrating other projects/targets (as mentioned before), and then we should look for ways that allow specification of a tsconfig. I like the idea of a migrate command, but that would be something to check with the CLI team. For now I could imagine us providing a ng generate schematic entry-point. e.g. ng generate @angular/cdk:migrations-v12 --tsConfig=<..> or something around that.

[crisbeto]

I'm closing this one since the v13 migrations are long gone by now.