fix(ngcc): do not fail hard when a format-path points to a non-existing or empty file by gkalpak · Pull Request #40985 · angular/angular

gkalpak · 2021-02-24T18:48:29Z

Previously, when ngcc encountered an entry-point with a format-path that pointed to a non-existing or empty file it would throw an error and stop processing the remaining tasks.

In the past, we used to ignore such format-paths and continue processing the rest of the tasks (see code). This was changed to a hard failure in 2954d1b. Looking at the code history, the reason for changing the behavior was an (incorrect) assumption that the condition could not fail. This assumption failed to take into account the case where a 3rd-party library has an invalid format-path in its package.json. This is an issue with the library, but it should not prevent ngcc from processing other packages/entry-points/formats.

This commit fixes this by reporting the task as failed but not throwing an error, thus allowing ngcc to continue processing other tasks.

Fixes #40965.

mary-poppins · 2021-02-24T18:59:34Z

You can preview 0b49aee at https://pr40985-0b49aee.ngbuilds.io/.

mary-poppins · 2021-02-24T20:50:45Z

You can preview e8a341a at https://pr40985-e8a341a.ngbuilds.io/.

…ng or empty file Previously, when `ngcc` encountered an entry-point with a format-path that pointed to a non-existing or empty file it would throw an error and stop processing the remaining tasks. In the past, we used to ignore such format-paths and continue processing the rest of the tasks ([see code][1]). This was changed to a hard failure in 2954d1b. Looking at the code history, the reason for changing the behavior was an (incorrect) assumption that the condition could not fail. This assumption failed to take into account the case where a 3rd-party library has an invalid format-path in its `package.json`. This is an issue with the library, but it should not prevent `ngcc` from processing other packages/entry-points/formats. This commit fixes this by reporting the task as failed but not throwing an error, thus allowing `ngcc` to continue processing other tasks. [1]: https://github.com/angular/angular/blob/3077c9a1f89c5bd75fb96c16e/packages/compiler-cli/ngcc/src/main.ts#L124 Fixes angular#40965

mary-poppins · 2021-02-25T09:45:49Z

You can preview d0a6348 at https://pr40985-d0a6348.ngbuilds.io/.

petebacondarwin · 2021-02-25T10:37:46Z

packages/compiler-cli/ngcc/src/execution/create_compile_function.ts

-            `${formatProperty} (formatPath: ${formatPath} | format: ${format})`);
+        onTaskCompleted(
+            task, TaskProcessingOutcome.Failed,
+            `property '${formatProperty}' pointing to a missing or empty file (${formatPath}).`);


Subjective NIT:

Suggested change

`property '${formatProperty}' pointing to a missing or empty file (${formatPath}).`);

`property \`${formatProperty}\` pointing to a missing or empty file: ${formatPath}.`);

petebacondarwin · 2021-02-25T10:38:53Z

packages/compiler-cli/ngcc/src/execution/tasks/completion.ts

 function createErrorMessage(fs: ReadonlyFileSystem, task: Task, message: string|null): string {
-  const jsFormat =
-      `${task.formatProperty} as ${getEntryPointFormat(fs, task.entryPoint, task.formatProperty)}`;
+  const jsFormat = `${task.formatProperty} as ${


Subjective NIT:

Suggested change

const jsFormat = `${task.formatProperty} as ${

const jsFormat = `\`${task.formatProperty}\` as ${

This seems out of scope for this PR.
(Also, you do realize this is your code, right? 😛)

Given that you are changing the line anyway. (And I have never been one to ideologically stick with my previous decisions).

petebacondarwin · 2021-02-25T10:40:46Z

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts

+      const logger = new MockLogger();
+      mainNgcc({
+        basePath: '/dist',
+        propertiesToConsider: ['es2015', 'fesm5', 'main'],


Perhaps put the main property earlier in the list so that we can see that it failure does not block other formats from being processed.

Also, what happens if the main property is the one that is assigned to the typings processing?

Also, what happens if the main property is the one that is assigned to the typings processing?

Not great things 😁

Hopefully, since main is low in the list of default properties to consider (and in CLI's list of properties), main will rarely be associated with processing typings.

In any case, this is not much different from what would happen if the formatPath for another property (which is associated with typings processing) points to a missing/empty file. In fact, ngcc seems to fail silently (from a super-quick loog) in that case, which is argueably worse than failing the taks and printing an error message 😁

mary-poppins · 2021-02-25T15:11:21Z

You can preview fcb6051 at https://pr40985-fcb6051.ngbuilds.io/.

mary-poppins · 2021-02-25T15:28:17Z

You can preview f82d92b at https://pr40985-f82d92b.ngbuilds.io/.

…-existing or empty file

mary-poppins · 2021-02-25T15:53:57Z

You can preview 50265ed at https://pr40985-50265ed.ngbuilds.io/.

…ng or empty file (#40985) Previously, when `ngcc` encountered an entry-point with a format-path that pointed to a non-existing or empty file it would throw an error and stop processing the remaining tasks. In the past, we used to ignore such format-paths and continue processing the rest of the tasks ([see code][1]). This was changed to a hard failure in 2954d1b. Looking at the code history, the reason for changing the behavior was an (incorrect) assumption that the condition could not fail. This assumption failed to take into account the case where a 3rd-party library has an invalid format-path in its `package.json`. This is an issue with the library, but it should not prevent `ngcc` from processing other packages/entry-points/formats. This commit fixes this by reporting the task as failed but not throwing an error, thus allowing `ngcc` to continue processing other tasks. [1]: https://github.com/angular/angular/blob/3077c9a1f89c5bd75fb96c16e/packages/compiler-cli/ngcc/src/main.ts#L124 Fixes #40965 PR Close #40985

angular-automatic-lock-bot · 2021-03-29T16:14:47Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

gkalpak added type: bug/fix state: WIP target: patch This PR is targeted for the next patch release comp: ngcc labels Feb 24, 2021

ngbot bot modified the milestone: Backlog Feb 24, 2021

google-cla bot added the cla: yes label Feb 24, 2021

gkalpak mentioned this pull request Feb 24, 2021

ngcc fails and produces a confusing error when a library points to a non-existent file #40965

Closed

gkalpak force-pushed the fix-ngcc-missing-format-path branch from 0b49aee to e8a341a Compare February 24, 2021 20:42

gkalpak force-pushed the fix-ngcc-missing-format-path branch from e8a341a to d0a6348 Compare February 25, 2021 09:37

gkalpak marked this pull request as ready for review February 25, 2021 10:01

gkalpak requested review from JoostK and petebacondarwin February 25, 2021 10:01

gkalpak added action: merge The PR is ready for merge by the caretaker and removed state: WIP labels Feb 25, 2021

petebacondarwin suggested changes Feb 25, 2021

View reviewed changes

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Feb 25, 2021

gkalpak force-pushed the fix-ngcc-missing-format-path branch from 55a33b7 to fcb6051 Compare February 25, 2021 15:03

gkalpak requested a review from petebacondarwin February 25, 2021 15:03

gkalpak force-pushed the fix-ngcc-missing-format-path branch from fcb6051 to f82d92b Compare February 25, 2021 15:20

fixup! fix(ngcc): do not fail hard when a format-path points to a non…

50265ed

…-existing or empty file

gkalpak force-pushed the fix-ngcc-missing-format-path branch from f82d92b to 50265ed Compare February 25, 2021 15:44

JoostK approved these changes Feb 25, 2021

View reviewed changes

gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 25, 2021

This comment has been minimized.

Sign in to view

petebacondarwin approved these changes Feb 25, 2021

View reviewed changes

zarend closed this in 284af73 Feb 26, 2021

gkalpak deleted the fix-ngcc-missing-format-path branch February 26, 2021 17:02

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 29, 2021

	`property '${formatProperty}' pointing to a missing or empty file (${formatPath}).`);
	`property \`${formatProperty}\` pointing to a missing or empty file: ${formatPath}.`);

	const jsFormat = `${task.formatProperty} as ${
	const jsFormat = `\`${task.formatProperty}\` as ${

Conversation

gkalpak commented Feb 24, 2021

Uh oh!

mary-poppins commented Feb 24, 2021

Uh oh!

mary-poppins commented Feb 24, 2021

Uh oh!

mary-poppins commented Feb 25, 2021

Uh oh!

petebacondarwin Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

gkalpak Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

gkalpak Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Feb 25, 2021

Uh oh!

mary-poppins commented Feb 25, 2021

Uh oh!

mary-poppins commented Feb 25, 2021

Uh oh!

This comment has been minimized.

angular-automatic-lock-bot bot commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants