perf: allow assertions to be removed in production mode #20146

crisbeto · 2020-07-31T13:02:18Z

Wraps all assertions in ngDevMode checks so that they can be stripped away in production mode. Furthermore, changes all the places where we were using the old isDevMode check.

src/youtube-player/BUILD.bazel

jelbourn

Overall looks good, I'll do a preliminary presubmit. FWIW, I probably would have preferred to make this change in smaller parts.

src/youtube-player/BUILD.bazel

crisbeto · 2020-08-01T10:05:32Z

Changed to dev_mode_types based on the discussion.

devversion

LGTM. One question about View Engine support.

devversion · 2020-08-03T07:12:29Z

src/cdk/drag-drop/directives/drop-list.ts

          const correspondingDropList = CdkDropList._dropLists.find(list => list.id === drop);

-          if (!correspondingDropList && isDevMode()) {
+          if (!correspondingDropList && (typeof ngDevMode === 'undefined' || ngDevMode)) {


Does removing isDevMode() mean that View Engine apps will have warnings/errors as in dev mode? It looks like View Engine enableProdMode does not set ngDevMode at all. Maybe it should?

Ah, I assumed that it would. We would need to fix that since we still need to report these issues in ViewEngine.

I believe that enableProdMode doesn't set the variable, because it's supposed to be replaced during build time. I don't think that this will be an issue for ViewEngine apps, because the majority of the current checks were running in production too. We have a handful of isDevMode ones, but they're sanity checks so it wouldn't really be a problem if they ran in production. The only one that may be a problem is the theme check in the common module since it has a getComputedStyle call so I'll add an exception for it.

src/material/core/common-behaviors/common-module.ts

jelbourn

LGTM

Caretaker note: let's be sure to test this explicitly in an internal app to make sure the warnings are still logged as expected

jelbourn · 2020-08-13T17:42:08Z

Caretaker note: hold off on merging this until Aug 17th so we can make a heads up announcement internally

jelbourn · 2020-08-13T23:38:09Z

@crisbeto would you mind rebasing? I want to peek at how much this reduces bundle size by

crisbeto · 2020-08-14T06:32:34Z

Rebased @jelbourn. Note that the tests are going to fail now, because #20304 hasn't been merged yet.

crisbeto · 2020-08-14T21:01:45Z

Re-rebased to fix the test failures.

jelbourn · 2020-08-14T22:46:42Z

src/material/sidenav/drawer.ts

-      if (drawer.position == 'end') {
-        if (this._end != null) {
-          throwMatDuplicatedDrawerError('end');
+    if (typeof ngDevMode === 'undefined' || ngDevMode) {


@crisbeto just noticed this one is actually gating actual behavior for the component (setting this._end or this._start)

ops, good catch. Fixed, although it does reveal that if we have some faulty logic, our tests won't catch it since they're only being run in dev mode.

jelbourn · 2020-08-14T22:58:13Z

Also I tested our internal component gallery app with this and it reduced the production bundle size by 4620 bytes.

Wraps all assertions in `ngDevMode` checks so that they can be stripped away in production mode. Furthermore, changes all the places where we were using the old `isDevMode` check.

angular-automatic-lock-bot · 2020-09-21T00:10:32Z

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

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 31, 2020

crisbeto force-pushed the ng-dev-mode branch 2 times, most recently from cf1e243 to 8f3416b Compare July 31, 2020 13:11

crisbeto marked this pull request as ready for review July 31, 2020 13:59

crisbeto requested review from a team, andrewseguin, devversion, jelbourn and mmalerba as code owners July 31, 2020 13:59

crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Jul 31, 2020

crisbeto assigned jelbourn and devversion Jul 31, 2020

devversion reviewed Jul 31, 2020

View reviewed changes

src/youtube-player/BUILD.bazel Outdated Show resolved Hide resolved

jelbourn reviewed Jul 31, 2020

View reviewed changes

src/youtube-player/BUILD.bazel Outdated Show resolved Hide resolved

crisbeto force-pushed the ng-dev-mode branch from 8f3416b to f483a00 Compare August 1, 2020 10:05

devversion approved these changes Aug 3, 2020

View reviewed changes

jelbourn reviewed Aug 3, 2020

View reviewed changes

src/material/core/common-behaviors/common-module.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the ng-dev-mode branch 2 times, most recently from cbfcffc to 643217b Compare August 3, 2020 19:21

jelbourn approved these changes Aug 3, 2020

View reviewed changes

jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: merge The PR is ready for merge by the caretaker labels Aug 3, 2020

andrewseguin added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Aug 11, 2020

crisbeto force-pushed the ng-dev-mode branch from 643217b to 5f34072 Compare August 11, 2020 19:46

crisbeto force-pushed the ng-dev-mode branch from 5f34072 to 10a2348 Compare August 14, 2020 06:32

AndrewKushnir mentioned this pull request Aug 14, 2020

perf(forms): use ngDevMode to tree-shake error messages angular/angular#37821

Closed

14 tasks

crisbeto force-pushed the ng-dev-mode branch from 10a2348 to 0d7db6d Compare August 14, 2020 21:01

crisbeto mentioned this pull request Aug 14, 2020

fix(cdk-experimental/menu): do not allow two separate triggers to open the same menu #20300

Merged

crisbeto force-pushed the ng-dev-mode branch from 0d7db6d to 32d31c7 Compare August 14, 2020 22:18

jelbourn reviewed Aug 14, 2020

View reviewed changes

crisbeto force-pushed the ng-dev-mode branch 2 times, most recently from ac96786 to 1ea40c8 Compare August 19, 2020 19:19

perf: allow assertions to be removed in production mode

2c8707a

Wraps all assertions in `ngDevMode` checks so that they can be stripped away in production mode. Furthermore, changes all the places where we were using the old `isDevMode` check.

crisbeto force-pushed the ng-dev-mode branch from 1ea40c8 to 2c8707a Compare August 20, 2020 03:38

wagnermaciel 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 Aug 21, 2020

wagnermaciel merged commit 79e4d28 into angular:master Aug 21, 2020

wagnermaciel mentioned this pull request Aug 24, 2020

Patch fixes for latest sync #20401

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 21, 2020

perf: allow assertions to be removed in production mode #20146

perf: allow assertions to be removed in production mode #20146

Uh oh!

Conversation

crisbeto commented Jul 31, 2020

Uh oh!

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crisbeto commented Aug 1, 2020

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

devversion Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

jelbourn Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

crisbeto Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Aug 13, 2020

Uh oh!

jelbourn commented Aug 13, 2020

Uh oh!

crisbeto commented Aug 14, 2020

Uh oh!

crisbeto commented Aug 14, 2020

Uh oh!

jelbourn Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

crisbeto Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Aug 14, 2020

Uh oh!

angular-automatic-lock-bot bot commented Sep 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants