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

ramthir · 2021-11-19T03:05:54Z

Long error messages can be tree-shaken in the production build.
Doing this, we will keep the throw and remove the long error messages.

See: #44210 (review)

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Long error messages are not tree-shakable in the production build.

Issue Number: N/A

What is the new behavior?

Long error messages are made tree-shakable in the production build based on the suggestion by @AndrewKushnir in PR 44210.

For constancy, I've done this change on all the error messages in application_ref.ts. @AndrewKushnir, please check if it's necessary.

Does this PR introduce a breaking change?

Yes
No

Other information

AndrewKushnir

@ramthir thanks for creating this PR 👍 The changes look good, I've just added a couple comments.

I've also noticed that the CI is failing and the error message indicates the positive impact of this change: bundles for the test apps decreased, see this CI job output:

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

So this change helped reduce the bundle size by ~800 bytes (which is great!). Could you please take a look at the failing CI jobs and update payload size limits according to the actual bundle sizes, mentioned in the CI output? Please let me know if any additional information is needed on this.

packages/core/src/application_ref.ts

AndrewKushnir

@ramthir thanks for resolving merge conflicts! We've discussed this PR with the team and I've left a couple comments with some feedback. Thank you.

packages/core/src/application_ref.ts

AndrewKushnir

@ramthir thanks for updating the PR, the changes look very good and we are very close. I've just added a couple more comments.

packages/core/src/application_ref.ts

AndrewKushnir

Thanks for applying the necessary updates @ramthir 👍

The changes look good and I've also rebased the PR (the setup CI job was failing). We'll need to get one more review on this PR and run extra tests in Google's codebase (I will take care of this). I will keep you updated on how it goes.

AndrewKushnir

Reviewed-for: size-tracking

AndrewKushnir

Reviewed-for: size-tracking

@ramthir it looks like the test CI job is failing with size-related reasons:

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

Could you please update the golden as I mentioned in the comment below?

goldens/size-tracking/integration-payloads.json

packages/core/src/application_ref.ts

Long error messages can be tree-shaken in the production build. Doing this, we will keep the throw and remove the long error messages. See: #44210 (review)

AndrewKushnir · 2021-11-30T05:03:28Z

One more run of tests in Google's codebase (internal-only link).

dylhunn

reviewed-for: size-tracking

dylhunn · 2021-11-30T17:27:10Z

@AndrewKushnir The presubmit you ran has one new failure (a hume target) -- have you looked at that at all? It seems to be unrelated to this change so it's likely a flake

jessicajaniuk

LGTM 🍪

reviewed-for: size-tracking

jessicajaniuk · 2021-11-30T18:03:42Z

packages/core/src/application_ref.ts

-    throw new Error(
-        'There can be only one platform. Destroy the previous one to create a new one.');
+    const errorMessage = (typeof ngDevMode === 'undefined' || ngDevMode) ?
+        'There can be only one platform. Destroy the previous one to create a new one.' :


I know you didn't write this line, but I hope whomever did write it initially was referencing Highlander.

AndrewKushnir · 2021-11-30T19:03:02Z

The presubmit you ran has one new failure (a hume target) -- have you looked at that at all? It seems to be unrelated to this change so it's likely a flake

@dylhunn yeah, that was a flake, I've restarted that test and everything is now fully "green" and this PR is ready for merge.

dylhunn · 2021-11-30T19:17:41Z

This PR was merged into the repository by commit 96dfc7a.

Long error messages can be tree-shaken in the production build. Doing this, we will keep the throw and remove the long error messages. See: #44210 (review) PR Close #44219

AndrewKushnir · 2021-12-01T02:10:30Z

@ramthir thanks for contributing to Angular, this change will have a positive impact on all Angular applications (the ApplicationRef class is used under the hood for all apps)! 👍

If you are interested in creating more PRs, there are some other places that would benefit from a similar change, for example:

Also a few places in the following files (search for throw new Error):

packages/core/src/di/injector_compatibility.ts
packages/core/src/di/r3_injector.ts
packages/core/src/render3/view_ref.ts
packages/core/src/render3/features/inherit_definition_feature.ts
packages/core/src/render3/i18n/i18n_apply.ts
packages/core/src/sanitization/sanitization.ts

I'm also exploring a possible solution to make the RuntimeError class reusable in other packages (currently it's only available in core) and will create a PR at some point.

ramthir · 2021-12-01T02:38:22Z

Thanks a lot, @AndrewKushnir; I'll look up and raise PRs on them.

Long error messages can be tree-shaken in the production build. Doing this, we will keep the throw and remove the long error messages. See: angular#44210 (review) PR Close angular#44219

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

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

google-cla bot added the cla: yes label Nov 19, 2021

pullapprove bot requested a review from alxhub November 19, 2021 03:06

AndrewKushnir requested review from AndrewKushnir and removed request for alxhub November 19, 2021 16:44

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 area: core Issues related to the framework runtime labels Nov 19, 2021

ngbot bot added this to the Backlog milestone Nov 19, 2021

AndrewKushnir reviewed Nov 19, 2021

View reviewed changes

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

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

ramthir requested a review from AndrewKushnir November 19, 2021 19:21

AndrewKushnir reviewed Nov 24, 2021

View reviewed changes

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

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

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

ramthir commented Nov 25, 2021

View reviewed changes

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

ramthir requested a review from AndrewKushnir November 25, 2021 04:30

AndrewKushnir reviewed Nov 25, 2021

View reviewed changes

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

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

ramthir requested a review from AndrewKushnir November 25, 2021 07:46

AndrewKushnir approved these changes Nov 26, 2021

View reviewed changes

pullapprove bot requested review from alxhub, atscott, dylhunn and IgorMinar November 26, 2021 18:11

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Nov 26, 2021

AndrewKushnir requested a review from jessicajaniuk November 26, 2021 18:14

AndrewKushnir approved these changes Nov 26, 2021

View reviewed changes

AndrewKushnir reviewed Nov 29, 2021

View reviewed changes

goldens/size-tracking/integration-payloads.json Outdated Show resolved Hide resolved

AndrewKushnir reviewed Nov 30, 2021

View reviewed changes

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

refactor(core): make the error messages tree shakable

675147e

Long error messages can be tree-shaken in the production build. Doing this, we will keep the throw and remove the long error messages. See: #44210 (review)

dylhunn approved these changes Nov 30, 2021

View reviewed changes

jessicajaniuk approved these changes Nov 30, 2021

View reviewed changes

AndrewKushnir removed request for alxhub, atscott and IgorMinar November 30, 2021 18:51

AndrewKushnir removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Nov 30, 2021

AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Nov 30, 2021

dylhunn closed this in 96dfc7a Nov 30, 2021

ramthir deleted the application-ref-throw-tree-shaking branch November 30, 2021 23:58

AndrewKushnir added the area: performance label Dec 1, 2021

ramthir mentioned this pull request Dec 3, 2021

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

Closed

14 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ramthir commented Nov 19, 2021

AndrewKushnir left a comment

AndrewKushnir left a comment

AndrewKushnir left a comment

AndrewKushnir left a comment •

edited

AndrewKushnir left a comment

AndrewKushnir left a comment

AndrewKushnir commented Nov 30, 2021

dylhunn left a comment

dylhunn commented Nov 30, 2021 •

edited

jessicajaniuk left a comment

jessicajaniuk Nov 30, 2021

AndrewKushnir commented Nov 30, 2021

dylhunn commented Nov 30, 2021

AndrewKushnir commented Dec 1, 2021

ramthir commented Dec 1, 2021

angular-automatic-lock-bot bot commented Jan 1, 2022

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

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

Conversation

ramthir commented Nov 19, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment • edited

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Nov 30, 2021

dylhunn left a comment

Choose a reason for hiding this comment

dylhunn commented Nov 30, 2021 • edited

jessicajaniuk left a comment

Choose a reason for hiding this comment

jessicajaniuk Nov 30, 2021

Choose a reason for hiding this comment

AndrewKushnir commented Nov 30, 2021

dylhunn commented Nov 30, 2021

AndrewKushnir commented Dec 1, 2021

ramthir commented Dec 1, 2021

angular-automatic-lock-bot bot commented Jan 1, 2022

AndrewKushnir left a comment •

edited

dylhunn commented Nov 30, 2021 •

edited