Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): check global ngDevMode is defined before checking value #32079

Closed

Conversation

@filipesilva
Copy link
Member

commented Aug 9, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Fix #31595

What is the new behavior?

ngDevMode is defined without being a toplevel side effect.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@filipesilva filipesilva requested review from mhevery, IgorMinar and clydin Aug 9, 2019
@filipesilva filipesilva requested review from angular/fw-core as code owners Aug 9, 2019
@googlebot googlebot added the cla: yes label Aug 9, 2019
@filipesilva

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Note for reviewers: the main changes are in packages/core/src/util/ng_dev_mode.ts.

Copy link
Member

left a comment

LGTM for ngcc

// TODO(misko): uncomment the next line once `ngDevMode` works with closure.
// if(ngDevMode) debugger;
// TODO(misko): uncomment the next line once `getNgDevMode()` works with closure.
// if(getNgDevMode()) debugger;

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 9, 2019

Member

Will getNgDevMode() work with closure?

This comment has been minimized.

Copy link
@filipesilva

filipesilva Aug 9, 2019

Author Member

No idea. It seemed to not work with ngDevMode either.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 12, 2019

Member

it should.

@@ -18,8 +18,8 @@ export const EMPTY_OBJ: {} = {};
export const EMPTY_ARRAY: any[] = [];

// freezing the values prevents any code from accidentally inserting new values in
if (typeof ngDevMode !== 'undefined' && ngDevMode) {
// These property accesses can be ignored because ngDevMode will be set to false
if (typeof getNgDevMode() !== 'undefined' && getNgDevMode()) {

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 9, 2019

Member

Is the typeof getNgDevMode() !== 'undefined' needed still?

This comment has been minimized.

Copy link
@filipesilva

filipesilva Aug 9, 2019

Author Member

No, I think not. Nice catch!

@mary-poppins

This comment has been minimized.

Copy link

commented Aug 9, 2019

@filipesilva filipesilva force-pushed the filipesilva:ngdevmode-side-effects branch 2 times, most recently from 1d9bb3f to a15e75a Aug 9, 2019
@mary-poppins

This comment has been minimized.

Copy link

commented Aug 9, 2019

@filipesilva

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Blocked on rollup/rollup#3044, this syntax change is causing the side effect test and golden symbol tests to erroneously retain code.

@filipesilva filipesilva force-pushed the filipesilva:ngdevmode-side-effects branch from a15e75a to 8f4ef0d Aug 9, 2019
@mary-poppins

This comment has been minimized.

Copy link

commented Aug 9, 2019

@@ -479,7 +479,7 @@ runInEachFileSystem(() => {
}
function compileNgModuleFactory__POST_R3__(injector, options, moduleType) {
ngDevMode && assertNgModuleType(moduleType);
getNgDevMode() && assertNgModuleType(moduleType);

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 12, 2019

Member

sorry for the nit pick, but I would prefer isNgDevMode()

// TODO(misko): uncomment the next line once `ngDevMode` works with closure.
// if(ngDevMode) debugger;
// TODO(misko): uncomment the next line once `getNgDevMode()` works with closure.
// if(getNgDevMode()) debugger;

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 12, 2019

Member

it should.

import {OnDestroy} from '../interface/lifecycle_hooks';
import {Type} from '../interface/type';
import {throwCyclicDependencyError, throwInvalidProviderError, throwMixedMultiProviderError} from '../render3/errors';
import {deepForEach} from '../util/array_utils';
import {getNgDevMode} from '../util/ng_dev_mode';

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 12, 2019

Member

love the explicit import!

* as much early warning and errors as possible.
*
* NOTE: changes to the `ngDevMode` name must be synced with `compiler-cli/src/tooling.ts`.
*/
if (typeof ngDevMode === 'undefined' || ngDevMode) {
ngDevModeResetPerfCounters();
export function getNgDevMode() {

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 12, 2019

Member

What if the getNgDevMode instead of being a function would be a constant such as:

export const isNgDevMode = (typeof ngDevMode === 'undefined' || ngDevMode) && ngDevModeResetPerfCounters();

This way it should still be:

  1. explicit import
  2. should be tree shakable
  3. does not need function in-lining, so it should have better warmup characteristics.

This comment has been minimized.

Copy link
@filipesilva

filipesilva Aug 12, 2019

Author Member

I think that would work, and be a nicer approach overall.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 28, 2019

Member

I don't think this would work. Imagine a case where ngDevMode is imported across chunks. In such a case the optimizer would not be able to tree shake this statements.

@filipesilva filipesilva force-pushed the filipesilva:ngdevmode-side-effects branch from 8f4ef0d to 4a450a5 Aug 12, 2019
@mary-poppins

This comment has been minimized.

Copy link

commented Aug 12, 2019

@filipesilva filipesilva force-pushed the filipesilva:ngdevmode-side-effects branch from 4a450a5 to 1d42191 Aug 12, 2019
* found in the LICENSE file at https://angular.io/license
*/

export interface NgDevModePerfCounters {

This comment has been minimized.

Copy link
@filipesilva

filipesilva Aug 12, 2019

Author Member

Moved the interface to avoid a circular dependency (ng_dev_mode.ts -> captured_globals.ts -> ng_dev_mode.ts).

@@ -0,0 +1,26 @@

This comment has been minimized.

Copy link
@filipesilva

filipesilva Aug 12, 2019

Author Member

This file should also capture ngI18nClosureMode and localize in the future, providing similar usage patterns as ngDevMode in this PR.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 12, 2019

Member

agreed.

@mary-poppins

This comment has been minimized.

Copy link

commented Aug 12, 2019

@filipesilva filipesilva force-pushed the filipesilva:ngdevmode-side-effects branch 2 times, most recently from 8093353 to 0c2d747 Aug 12, 2019
@mary-poppins

This comment has been minimized.

Copy link

commented Aug 12, 2019

@AndrewKushnir AndrewKushnir added cla: yes and removed cla: no labels Sep 11, 2019
@googlebot

This comment has been minimized.

Copy link

commented Sep 11, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

One more presubmit, to make sure g3 checks are "green":

@alan-agius4 alan-agius4 force-pushed the filipesilva:ngdevmode-side-effects branch from d4fedc2 to 3597281 Sep 11, 2019
@googlebot

This comment has been minimized.

Copy link

commented Sep 11, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 11, 2019
@AndrewKushnir AndrewKushnir added cla: yes and removed cla: no labels Sep 11, 2019
@mary-poppins

This comment has been minimized.

Copy link

commented Sep 11, 2019

@googlebot

This comment has been minimized.

Copy link

commented Sep 11, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

FYI, most recent VE and Ivy presubmits are successful.

@kara
kara approved these changes Sep 11, 2019
Copy link
Contributor

left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

merge-assistance: global approval

@matsko

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@filipesilva this fails on patch.

@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 13, 2019

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 Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.