X Tutup
Skip to content

fix: make sure that Zone does not show up in angular2.d.ts #7655

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.

Sign up for GitHub

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

Closed
mhevery wants to merge 2 commits into from
Closed

fix: make sure that Zone does not show up in angular2.d.ts #7655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9a49af
chore: re-enable all tests (accidental ddescribe checkin)
mhevery Mar 18, 2016
17c5193
fix: make sure that Zone does not show up in angular2.d.ts
mhevery Mar 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,7 +988,7 @@ gulp.task('test.typings',
['!pre.test.typings.layoutNodeModule', '!pre.test.typings.copyTypingsSpec'], function() {
var tsc = require('gulp-typescript');

return gulp.src([tmpdir + '/*.ts', 'node_modules/zone.js/dist/zone.js.d.ts'])
return gulp.src([tmpdir + '/*.ts'])
Copy link
Copy Markdown
Contributor

@alexeagle alexeagle Mar 18, 2016

Choose a reason for hiding this comment

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

oh, this is why the test was passing.

.pipe(tsc({
target: 'ES6',
module: 'commonjs',
Expand Down
91 changes: 47 additions & 44 deletions modules/angular2/src/core/zone/ng_zone_impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@ export class NgZoneError {
}


export class NgZoneImpl implements ZoneSpec {
export class NgZoneImpl {
static isInAngularZone(): boolean { return Zone.current.get('isAngularZone') === true; }

public name: string = 'angular';
public properties: {[k: string]: string} = <any>{'isAngularZone': true};

/** @internal */
private outer: Zone;
/** @internal */
private inner: Zone;

private onEnter: () => void;
Expand All @@ -37,60 +36,64 @@ export class NgZoneImpl implements ZoneSpec {
this.setMacrotask = setMacrotask;
this.onError = onError;

if (global.Zone) {
if (Zone) {
this.outer = this.inner = Zone.current;
if (Zone['wtfZoneSpec']) {
this.inner = this.inner.fork(Zone['wtfZoneSpec']);
}
if (trace) {
this.inner = this.inner.fork(Zone['longStackTraceZoneSpec']);
}
this.inner = this.inner.fork(this);
} else {
throw new Error('Angular2 needs to be run with Zone.js polyfill.');
}
}
this.inner = this.inner.fork({
Copy link
Copy Markdown
Contributor

@alexeagle alexeagle Mar 18, 2016

Choose a reason for hiding this comment

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

nit: this gets pretty deeply nested, did you consider a private inner class that implements ZoneSpec?

Copy link
Copy Markdown
Contributor Author

@mhevery mhevery Mar 18, 2016

Choose a reason for hiding this comment

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

Yes, I thought about it, but this is cheaper in terms of number of bytes. If you feel strongly I can make the change.

Copy link
Copy Markdown
Contributor

@alexeagle alexeagle Mar 18, 2016

Choose a reason for hiding this comment

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

Nope, don't feel strongly.

On Thu, Mar 17, 2016 at 9:56 PM Miško Hevery notifications@github.com
wrote:

In modules/angular2/src/core/zone/ng_zone_impl.ts
#7655 (comment):

@@ -45,52 +44,56 @@ export class NgZoneImpl implements ZoneSpec {
if (trace) {
this.inner = this.inner.fork(Zone['longStackTraceZoneSpec']);
}

  •  this.inner = this.inner.fork(this);
  • } else {
  •  throw new Error('Angular2 needs to be run with Zone.js polyfill.');
  • }
  • }
  •  this.inner = this.inner.fork({

Yes, I thought about it, but this is cheaper in terms of number of bytes.
If you feel strongly I can make the change.

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
https://github.com/angular/angular/pull/7655/files/17c5193fc0b648d83b017c1a2badb29c3ea9a035#r56613191

name: 'angular',
properties:<any>{'isAngularZone': true},
onInvokeTask: (delegate: ZoneDelegate, current: Zone, target: Zone, task: Task,
applyThis: any, applyArgs: any): any => {
try {
this.onEnter();
return delegate.invokeTask(target, task, applyThis, applyArgs);
} finally {
this.onLeave();
}
},

onInvokeTask(delegate: ZoneDelegate, current: Zone, target: Zone, task: Task, applyThis: any,
applyArgs: any): any {
try {
this.onEnter();
return delegate.invokeTask(target, task, applyThis, applyArgs);
} finally {
this.onLeave();
}
};

onInvoke: (delegate: ZoneDelegate, current: Zone, target: Zone, callback: Function,
applyThis: any, applyArgs: any[], source: string): any => {
try {
this.onEnter();
return delegate.invoke(target, callback, applyThis, applyArgs, source);
} finally {
this.onLeave();
}
},

onInvoke(delegate: ZoneDelegate, current: Zone, target: Zone, callback: Function, applyThis: any,
applyArgs: any[], source: string): any {
try {
this.onEnter();
return delegate.invoke(target, callback, applyThis, applyArgs, source);
} finally {
this.onLeave();
}
}
onHasTask:
(delegate: ZoneDelegate, current: Zone, target: Zone, hasTaskState: HasTaskState) => {
delegate.hasTask(target, hasTaskState);
if (current == target) {
// We are only interested in hasTask events which originate from our zone
// (A child hasTask event is not interesting to us)
if (hasTaskState.change == 'microTask') {
this.setMicrotask(hasTaskState.microTask);
} else if (hasTaskState.change == 'macroTask') {
this.setMacrotask(hasTaskState.macroTask);
}
}
},

onHasTask(delegate: ZoneDelegate, current: Zone, target: Zone, hasTaskState: HasTaskState) {
delegate.hasTask(target, hasTaskState);
if (current == target) {
// We are only interested in hasTask events which originate from our zone
// (A child hasTask event is not interesting to us)
if (hasTaskState.change == 'microTask') {
this.setMicrotask(hasTaskState.microTask);
} else if (hasTaskState.change == 'macroTask') {
this.setMacrotask(hasTaskState.macroTask);
}
onHandleError: (delegate: ZoneDelegate, current: Zone, target: Zone, error: any):
boolean => {
delegate.handleError(target, error);
this.onError(new NgZoneError(error, error.stack));
return false;
}
});
} else {
throw new Error('Angular2 needs to be run with Zone.js polyfill.');
}
}

onHandleError(delegate: ZoneDelegate, current: Zone, target: Zone, error: any): boolean {
delegate.handleError(target, error);
this.onError(new NgZoneError(error, error.stack));
return false;
}

runInner(fn: () => any): any { return this.inner.runGuarded(fn); };
runOuter(fn: () => any): any { return this.outer.run(fn); };
}
3 changes: 1 addition & 2 deletions modules/angular2/src/facade/lang.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ export interface BrowserNodeGlobal {
Math: any; // typeof Math;
assert(condition: any): void;
Reflect: any;
Zone: typeof Zone;
getAngularTestability: Function;
getAllAngularTestabilities: Function;
getAllAngularRootElements: Function;
Expand Down Expand Up @@ -477,4 +476,4 @@ export function bitWiseAnd(values: number[]): number {

export function escape(s: string): string {
return _global.encodeURI(s);
}
}
2 changes: 1 addition & 1 deletion modules/angular2/test/i18n/message_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
import {Message, id} from 'angular2/src/i18n/message';

export function main() {
ddescribe('Message', () => {
describe('Message', () => {
Copy link
Copy Markdown
Contributor

@alexeagle alexeagle Mar 18, 2016

Choose a reason for hiding this comment

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

does this have anything to do with us missing the bad release? or are we still missing test coverage?

Copy link
Copy Markdown
Contributor Author

@mhevery mhevery Mar 18, 2016

Choose a reason for hiding this comment

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

It is a separate SHA. We were only running single test on Travis.

describe("id", () => {
it("should return a different id for messages with and without the meaning", () => {
let m1 = new Message("content", "meaning", null);
Expand Down
X Tutup