refactor: make RuntimeError class reusable across packages by AndrewKushnir · Pull Request #44398 · angular/angular

AndrewKushnir · 2021-12-08T01:55:30Z

This PR contains a few commits that make the RuntimeError class reusable across packages + performs some refactoring to use the class in the Forms package.

Note: this is a replacement PR for #44348.

PR Type

What kind of change does this PR introduce?

Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

Yes
No

AndrewKushnir · 2021-12-08T05:27:31Z

Hi @twerske, this PR updates the RuntimeError class (and related logic) to make it reusable across Angular packages. As a part of this change, I'm starting to use a reserved bit of the error code to differentiate Angular packages. The error code prefix used to be hardcoded as NG0 (see here) and now the digit that follows NG would be different per package. So the format for the error codes in the core package remains NG0xxx, when for forms it'd start with NG1xxx and NG2xxx for common (in the future we can use other digits for Router, etc).

Just wanted to check with you if this use of the error code bit is aligned with the idea that was implemented initially.

Thank you.

AndrewKushnir · 2021-12-08T05:32:01Z

@alxhub @dylhunn @jessicajaniuk thanks for your review in the previous version of this PR (#44348). I've created a new one with a slightly different approach. Instead of creating a new class for each package (as it was in #44348), this PR reuses the same class and each package contains a thin wrapper that supplies the package id. The motivation for this is to maintain easy error handling (try { ... } catch(e) { if (e instanceof RuntimeError) { ... } }) which wouldn't be possible if we create new classes per package.

Could you please review this PR when you get a chance?

jessicajaniuk

LGTM 🍪

reviewed-for: size-tracking, fw-core, fw-common

packages/common/src/errors.ts

dylhunn

reviewed-for: size-tracking, fw-core, fw-common, fw-forms

twerske · 2021-12-08T21:45:44Z

Thanks for tagging me @AndrewKushnir!

One design concern is that NG1 through NG9 are already taken by compile time errors so I'm not sure this can be designated and reassigned out per package as this PR introduces. You can see some of these error documented here: https://angular.io/errors/

For example NG1001: Decorator argument is not an object literal does not fit this new classification of forms

In the initial design for runtime errors (internal design can be found here) one of the constraints was that NG1-9 are already reserved for compiler errors so NG0 was proposed for all runtime errors and then the second digit indicates category.

Is there a way to add or use the categories in the second digit per package? Love the addition of additional error codes in a more standard format and if there's space to add docs as well that'd be AMAZING!

packages/core/src/application_ref.ts

AndrewKushnir · 2021-12-09T01:04:44Z

Thanks for the review everyone!

@twerske thanks for your feedback, that makes sense. I've reworked the changes a bit to retain the NG0 prefix and annotated errors.ts file in forms and common packages to mention which error code range should be used:

100-999 - is the range for core
1000-1999 - the range for forms
2000-2999 - for common
... similarly, we'll allocate ranges for other packages as well

The benefit is that we'll use the bit next to NG0 as a namespace (i.e. NG01xxx would be for forms, NG02xxx for common, etc). One downside is that the error code length for non-core errors would be 1 character longer.

This commit updates the code around the `RuntimeError` class to make it more reusable between packages (currently it's only usable inside the `core` package). Specifically: - the error formatting logic was updated to handle cases when there is no error message provided - there is no special Set that contains a set of error codes for which we have guides on angular.io. Instead, this is now encoded into the error code itself (making such codes negative integers). Having a separate Set makes it non-tree-shakable, which we want to avoid. This change should allow to employ the `RuntimeError` class in other packages to further standardize this subsystem and make the errors thrown by the framework consistent. As a part of the refactoring, the `common` package code was also updated to follow the same logic as `core`, since the `RuntimeError` class was used there as well.

This commit moves some logic to make the location of runtime error codes consistent across packages. Now all error codes are located in `packages/core/src/errors.ts` file.

AndrewKushnir · 2022-01-06T21:36:43Z

Note: like the compiler errors, we should guard the runtime error enums with a public-api test.

Agreed, I will followup with a new PR once this one lands.

AndrewKushnir · 2022-01-06T21:39:55Z

Note to the Caretaker: this PR would require a local mod update in g3, please ping me for additional info.

atscott · 2022-01-06T23:43:20Z

This PR was merged into the repository by commit 67df935.

This commit moves some logic to make the location of runtime error codes consistent across packages. Now all error codes are located in `packages/core/src/errors.ts` file. PR Close #44398

This commit performs some refactoring of the AbstractControl-based classes to employ shared `RuntimeError` class and also updates the code to avoid duplication and improve minification. PR Close #44398

…4398) This commit updates the code around the `RuntimeError` class to make it more reusable between packages (currently it's only usable inside the `core` package). Specifically: - the error formatting logic was updated to handle cases when there is no error message provided - there is no special Set that contains a set of error codes for which we have guides on angular.io. Instead, this is now encoded into the error code itself (making such codes negative integers). Having a separate Set makes it non-tree-shakable, which we want to avoid. This change should allow to employ the `RuntimeError` class in other packages to further standardize this subsystem and make the errors thrown by the framework consistent. As a part of the refactoring, the `common` package code was also updated to follow the same logic as `core`, since the `RuntimeError` class was used there as well. PR Close angular#44398

This commit moves some logic to make the location of runtime error codes consistent across packages. Now all error codes are located in `packages/core/src/errors.ts` file. PR Close angular#44398

This commit performs some refactoring of the AbstractControl-based classes to employ shared `RuntimeError` class and also updates the code to avoid duplication and improve minification. PR Close angular#44398

…44652) This commit updates the code around the `RuntimeError` class to make it more reusable between packages (currently it's only usable inside the `core` package). Specifically: - the error formatting logic was updated to handle cases when there is no error message provided - there is no special Set that contains a set of error codes for which we have guides on angular.io. Instead, this is now encoded into the error code itself (making such codes negative integers). Having a separate Set makes it non-tree-shakable, which we want to avoid. This change should allow to employ the `RuntimeError` class in other packages to further standardize this subsystem and make the errors thrown by the framework consistent. As a part of the refactoring, the `common` package code was also updated to follow the same logic as `core`, since the `RuntimeError` class was used there as well. PR Close #44398 PR Close #44652

This commit moves some logic to make the location of runtime error codes consistent across packages. Now all error codes are located in `packages/core/src/errors.ts` file. PR Close #44398 PR Close #44652

This commit performs some refactoring of the AbstractControl-based classes to employ shared `RuntimeError` class and also updates the code to avoid duplication and improve minification. PR Close #44398 PR Close #44652

Runtime error codes in the Core, Common and Forms packages were not included into the `public-api` group reviews. This commit creates the necessary golden files to keep track of further changes in the runtime codes. This is a followup from angular#44398 (comment).

Runtime error codes in the Core, Common and Forms packages were not included into the `public-api` group reviews. This commit creates the necessary golden files to keep track of further changes in the runtime codes. This is a followup from #44398 (comment). PR Close #44677

Runtime error codes in the Core, Common and Forms packages were not included into the `public-api` group reviews. This commit creates the necessary golden files to keep track of further changes in the runtime codes. This is a followup from angular#44398 (comment). PR Close angular#44677

angular-automatic-lock-bot · 2022-02-06T16:06:15Z

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.}

AndrewKushnir added state: WIP refactoring Issue that involves refactoring or code-cleanup area: core Issues related to the framework runtime area: forms target: patch This PR is targeted for the next patch release labels Dec 8, 2021

ngbot bot modified the milestone: Backlog Dec 8, 2021

AndrewKushnir mentioned this pull request Dec 8, 2021

refactor: make RuntimeError class reusable across packages #44348

Closed

3 tasks

AndrewKushnir force-pushed the reuse-runtime-error branch from a975111 to 95ba577 Compare December 8, 2021 02:16

AndrewKushnir requested review from alxhub, dylhunn, jessicajaniuk and twerske December 8, 2021 05:11

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Dec 8, 2021

AndrewKushnir marked this pull request as ready for review December 8, 2021 05:12

pullapprove bot requested a review from jelbourn December 8, 2021 05:12

jessicajaniuk approved these changes Dec 8, 2021

View reviewed changes

packages/common/src/errors.ts Outdated Show resolved Hide resolved

dylhunn approved these changes Dec 8, 2021

View reviewed changes

diicar reviewed Dec 8, 2021

View reviewed changes

packages/core/src/application_ref.ts Outdated Show resolved Hide resolved

AndrewKushnir force-pushed the reuse-runtime-error branch 2 times, most recently from bd7acbe to 0aa574a Compare December 9, 2021 00:56

danduh approved these changes Dec 9, 2021

View reviewed changes

AndrewKushnir mentioned this pull request Dec 14, 2021

refactor(core): make the error messages tree shakable #44359

Closed

14 tasks

AndrewKushnir added 2 commits January 5, 2022 16:47

refactor(core): move runtime error code logic

0600dc6

This commit moves some logic to make the location of runtime error codes consistent across packages. Now all error codes are located in `packages/core/src/errors.ts` file.

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Jan 6, 2022

AndrewKushnir removed request for jelbourn and twerske January 6, 2022 21:36

AndrewKushnir added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jan 6, 2022

atscott added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 6, 2022

atscott closed this in 66e59bc Jan 6, 2022

AndrewKushnir mentioned this pull request Jan 7, 2022

[13.1.x] refactor: make RuntimeError class reusable across packages #44652

Closed

AndrewKushnir mentioned this pull request Jan 10, 2022

ci: add golden files for runtime error codes #44677

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: make RuntimeError class reusable across packages#44398

refactor: make RuntimeError class reusable across packages#44398
AndrewKushnir wants to merge 3 commits intoangular:masterfrom
AndrewKushnir:reuse-runtime-error

AndrewKushnir commented Dec 8, 2021

Uh oh!

AndrewKushnir commented Dec 8, 2021

Uh oh!

AndrewKushnir commented Dec 8, 2021

Uh oh!

jessicajaniuk left a comment

Uh oh!

Uh oh!

dylhunn left a comment

Uh oh!

twerske commented Dec 8, 2021

Uh oh!

Uh oh!

AndrewKushnir commented Dec 9, 2021 •

edited

Loading

Uh oh!

AndrewKushnir commented Jan 6, 2022

Uh oh!

AndrewKushnir commented Jan 6, 2022

Uh oh!

atscott commented Jan 6, 2022

Uh oh!

angular-automatic-lock-bot bot commented Feb 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

AndrewKushnir commented Dec 8, 2021

PR Type

Does this PR introduce a breaking change?

Uh oh!

AndrewKushnir commented Dec 8, 2021

Uh oh!

AndrewKushnir commented Dec 8, 2021

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dylhunn left a comment

Choose a reason for hiding this comment

Uh oh!

twerske commented Dec 8, 2021

Uh oh!

Uh oh!

AndrewKushnir commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir commented Jan 6, 2022

Uh oh!

AndrewKushnir commented Jan 6, 2022

Uh oh!

atscott commented Jan 6, 2022

Uh oh!

angular-automatic-lock-bot bot commented Feb 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

AndrewKushnir commented Dec 9, 2021 •

edited

Loading