Provide better story for individual migration results

[filipesilva]

This issue impacts 9.0.0-rc.0 and happened while updating https://github.com/johannesjo/super-productivity and https://github.com/SAP/cloud-commerce-spartacus-storefront.

A failed update will show a message saying both the individual migration and the update failed:

× Package install failed, see above.
The update failed. See above.
git HEAD was at 85f08a4d93e684566f751e38e2a2feffe0194cd9 before migrations.
×  Migration failed. See above for further details.

The update below did not finish successfully however, and instead just shows the last migration as successful (@devversion confirms this). In this case the migration shows an error that it seems to suggest was actually recovered from, but it fact the intent is that the migration was interrupted and should later be resumed.

>  Undecorated classes with DI migration.
   As of Angular 9, it is no longer supported to use Angular DI on a class that does not have an Angular decorator.
   Read more about this here: https://v9.angular.io/guide/migration-undecorated-classes
    
    This migration uses the Angular compiler internally and therefore projects that no longer build successfully after the update cannot run the migration. Please ensure there are 
no AOT compilation errors and rerun the migration.. The following project failed: projects/storefrontlib/tsconfig.lib.json
    
    TypeError: Cannot read property 'module' of undefined
    
    Could not migrate all undecorated classes that use dependency
    injection. Some project targets could not be analyzed due to
    TypeScript program failures.

    Migration can be rerun with: "ng update @angular/core --from 8.0.0 --to 9.0.0 --migrate-only"

√  Migration succeeded.

A completely successful migration will look the same as the migration above though:

kamik@RED-X1C6 MINGW64 /d/sandbox/latest-app (master)
$ ng update @angular/cli @angular/core --next
The installed Angular CLI version is older than the latest published version.
Installing a temporary version to perform the update.
Installing packages for tooling via yarn.
warning @angular-devkit/architect@0.900.0-rc.0: The engine "pnpm" appears to be invalid.
warning @schematics/angular@9.0.0-rc.0: The engine "pnpm" appears to be invalid.
warning @angular-devkit/schematics@9.0.0-rc.0: The engine "pnpm" appears to be invalid.
warning @angular-devkit/core@9.0.0-rc.0: The engine "pnpm" appears to be invalid.
warning @schematics/update@0.900.0-rc.0: The engine "pnpm" appears to be invalid.
warning @angular/cli@9.0.0-rc.0: The engine "pnpm" appears to be invalid.
Installed packages for tooling via yarn.
Using package manager: 'yarn'
Collecting installed dependencies...
Found 35 dependencies.
Fetching dependency metadata from registry...
    Updating package.json with dependency @angular/cli @ "9.0.0-rc.0" (was "8.3.17")...
    Updating package.json with dependency @angular/core @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/language-service @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/compiler-cli @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/animations @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/platform-browser @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/platform-browser-dynamic @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/common @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/compiler @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/forms @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular/router @ "9.0.0-rc.0" (was "8.2.13")...
    Updating package.json with dependency @angular-devkit/build-angular @ "0.900.0-rc.0" (was "0.803.17")...
UPDATE package.json (1636 bytes)
√ Packages installed successfully.
** Executing migrations of package '@angular/cli' **

>  Update an Angular CLI project to version 9.
UPDATE angular.json (6359 bytes)
UPDATE package.json (1637 bytes)
UPDATE server.ts (1863 bytes)
√ Packages installed successfully.
√  Migration succeeded.

>  Update lazy loading syntax to use dynamic imports.
√  Migration succeeded.

** Executing migrations of package '@angular/core' **

>  Static flag migration.
   Removes the `static` flag from dynamic queries.
   As of Angular 9, the "static" flag defaults to false and is no longer required for your view and content queries.
   Read more about this here: https://v9.angular.io/guide/migration-dynamic-flag
√  Migration succeeded.

>  Missing @Injectable migration.
   In Angular 9, enforcement of @Injectable decorators for DI is a bit stricter.
   Read more about this here: https://v9.angular.io/guide/migration-injectable
√  Migration succeeded.

>  ModuleWithProviders migration.
   In Angular 9, the ModuleWithProviders type without a generic has been deprecated.
   This migration adds the generic where it is missing.
   Read more about this here: https://v9.angular.io/guide/migration-module-with-providers
√  Migration succeeded.

>  NGCC postinstall migration.
   Adds an ngcc invocation to npm/yarn's postinstall script.
   Read more about this here: https://v9.angular.io/guide/migration-ngcc
UPDATE package.json (1743 bytes)
√ Packages installed successfully.
√  Migration succeeded.

>  Renderer to Renderer2 migration.
   As of Angular 9, the Renderer class is no longer available.
   Renderer2 should be used instead.
   Read more about this here: https://v9.angular.io/guide/migration-renderer
√  Migration succeeded.

>  Undecorated classes with decorated fields migration.
   As of Angular 9, it is no longer supported to have Angular field decorators on a class that does not have an Angular decorator.
   Read more about this here: https://v9.angular.io/guide/migration-undecorated-classes
√  Migration succeeded.

>  Undecorated classes with DI migration.
   As of Angular 9, it is no longer supported to use Angular DI on a class that does not have an Angular decorator.
   Read more about this here: https://v9.angular.io/guide/migration-undecorated-classes
√  Migration succeeded.

We should confirm the update finished, was successful, or if it was interrupted midway and should later be continued.

[devversion]

This was previously not an issue because the CLI did not print any message stating that the migration was successful or not. Now with V9 of the CLI, it always prints Migration successful. This is not correct since migrations can be simply incomplete or failing gracefully (for various reasons).

e.g. most of the time migrations have edge cases which cannot be handled automatically. In those cases user action is required and it does not make sense to make the user think that the migration was successful/complete.

[filipesilva]

If a migration can stop midway, it shouldn't print this indication though:

    Could not migrate all undecorated classes that use dependency
    injection. Some project targets could not be analyzed due to
    TypeScript program failures.

    Migration can be rerun with: "ng update @angular/core --from 8.0.0 --to 9.0.0 --migrate-only"

This is incorrect because many packages can be updated at the same time. In this example it is only correct to use --migrate-only with @angular/core if that was the last package to be updated. But the migration does not know this.

The update procedure itself should indicate how to resume from an interrupted migration, and show the full command to resume all un-fully-migrated packages.

[devversion]

I don't quite follow why that would be incorrect. The migration coming from @angular/core just asks the developer to re-run the migrations for @angular/core since the migration from core failed and manual action is needed. Previous migrations which were successful as part of @angular/core and will be scheduled twice if the given command is ran a second time, will just be a noop.

Agreed that this is something the CLI could do, but at the time of writing this is not available and we achieved something similar by printing such a message. We did this in version 8 too.

[filipesilva]

It's incorrect because the original command was ng update @angular/core @angular/cli --next. The migrations for @angular/cli might not have happened yet when the update process was interrupted.

So running ng update @angular/core --from 8.0.0 --to 9.0.0 --migrate-only does not get you to the same place as if the migration had not been interrupted. To do that you'd need instead to do ng update @angular/core @angular/cli --from 8.0.0 --to 9.0.0 --migrate-only (including CLI as well).

[devversion]

Why would the migrations from @angular/cli be relevant though for that particular migration that failed? It's true that the CLI migrations can change things too, but from the perspective of the @angular/core migration, the only prerequisite is that there is an TS project.

If you are saying that the @angular/core migrations are dependent on migrations of the CLI, then I'm not sure if that is the right thing. I'd expect the the core migrations to be not dependent on migrations outside of the @angular/core scope. In general though.

The way you are describing the issue is that followed migrations will be interrupted. This is not the goal and not the case currently. The goal is:

1. All followed migrations continue executing.
2. But only the incomplete migration will be re-run manually (with the given command)
   • Other migrations as part of @angular/core ran already and will be a noop.

[filipesilva]

Ah I see. Yes I did think that the last migration interrupted the process and prevented the remaining ones from executing.

So that does sound right. We should somehow indicate that some migrations need extra attention and might require action.

[alan-agius4]

The problem is that if you are handling an error gracefully, it means that the migration was successful. Because a migration fails when either a workflow event of type error or an exception were triggered.

My 2 cents on the way forward here would be;

• In the schematics, if there is an error, it should be thrown. As otherwise it will always be with a successful result.
• In the CLI when a one of the migrations fail we don't stop the other migrations. We continue running the other migrations.

[dgp1130]

Talked about this in the planning meeting today, decision for now is to simply rename "Migration succeeded." to "Migration completed." to be more clear that we don't necessarily know that it was successful. Sent out a PR to change this.

Longer term, we'll have to look in to a broader redesign which allows some migrations to fail and more clearly communicate them to the user (something like Karma where some tests pass and other fail).

[dgp1130]

Message change was merged to master and 9.0.x branches. I'll leave this issue open since it seems to be a larger challenge which will require a greater redesign to address. If that is being tracked elsewhere, then we can maybe close this issue.

[filipesilva]

Yeah this looks as good a place to track it as any.

[devversion]

@alan-agius4 I feel differently. An example is that a migration cannot handle all patterns automatically and requests the developer to manually take action. In those case the migration is not successful nor complete. This is actually quite common since migrations most of the time are limited by the possibilities of static analysis. I agree that we could rethink how errors are handled (e.g. not gracefully exiting), but in terms of this issue, it seems to unveil the general issue where the CLI makes an assumption. It's wrong to assume that a migration is complete or successful if it didn't throw. There should be an API to communicate back to the CLI what the status is (for purposes like printing how to rerun the migration). In any case though (except when an error is thrown), the CLI should not make any assumption IMO.

[clydin]

A larger expansion and refactoring is planned and will include a structured migration system. However, at the current juncture the migrations will need to work within the current confines of the system. This unfortunately means that the migrations themselves will need to contain more boilerplate and infrastructure than should ideally be needed. But this will improve as update is updated.