Skip to content

refactor: make RuntimeError class reusable across packages#44348

Closed
AndrewKushnir wants to merge 4 commits intoangular:masterfrom
AndrewKushnir:forms-tree-shaking
Closed

refactor: make RuntimeError class reusable across packages#44348
AndrewKushnir wants to merge 4 commits intoangular:masterfrom
AndrewKushnir:forms-tree-shaking

Conversation

@AndrewKushnir
Copy link
Contributor

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.

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 AndrewKushnir added state: WIP refactoring Issue that involves refactoring or code-cleanup hotlist: error messages area: core Issues related to the framework runtime area: forms labels Dec 2, 2021
@google-cla google-cla bot added the cla: yes label Dec 2, 2021
@ngbot ngbot bot modified the milestone: Backlog Dec 2, 2021
@AndrewKushnir
Copy link
Contributor Author

Based on the test CI job output, this change also brings ~1kb payload size reduction when Forms package is used:

FAIL: Commit undefined uncompressed main fell below expected size by 500 bytes or >1%
(expected: 163342, actual: 162275).

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 public-facing error code now includes a package id (it was hardcoded before)
- 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.
This commit moves some scripts around to co-locate RuntimeError-related scipts under `packages/core/src/errors` folder.
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.
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release and removed state: WIP labels Dec 3, 2021
Copy link
Contributor Author

@AndrewKushnir AndrewKushnir Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: this is a type-only import from util.ts -> index.ts, not a real cycle. We'd need to re-discuss if type-only imports should be excluded from the cycle detection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can inline the code from the util.ts into the index.ts to avoid this cycle, but I'd prefer not to do that as I'm trying to keep the errors/index.ts as small as possible as it contains the structure that I've reused across other packages.

@AndrewKushnir AndrewKushnir marked this pull request as ready for review December 3, 2021 02:48
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice wins on size!

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

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🍪

reviewed-for: size-tracking

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document that this minus sign is a code size optimization, so we don't end up shipping a Set or array to runtime.

Previously, the NgSwitch directive used RuntimeError class from core as is. As a result, it was only possible to reuse some existing error codes (which were core-specific, with core prefix in the code). This commit updates the logic to create package-specific version of the `RuntimeError` class that can be used in the common package. This commit also refactors existing code to use that new class.
@AndrewKushnir
Copy link
Contributor Author

Closing this PR in favor of #44398 where I've changed the implementation to avoid creating a new unique RuntimeError classes for each package (thanks Alex for the feedback on this!).

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime area: forms cla: yes hotlist: error messages refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants