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

feat(core): support for bootstrap with custom zone #17672

Closed
wants to merge 2 commits into from

Conversation

@jasonaden
Copy link
Contributor

commented Jun 21, 2017

This PR is based on Misko's PR (#17142), with PR comments addressed.

@googlebot

This comment has been minimized.

Copy link

commented Jun 21, 2017

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added the cla: no label Jun 21, 2017

@jasonaden jasonaden force-pushed the jasonaden:feat_no_ng_zone branch from 8b44910 to 2fbe119 Jun 21, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 21, 2017

The angular.io preview for 2fbe119 is available here.

@jasonaden jasonaden force-pushed the jasonaden:feat_no_ng_zone branch from 2fbe119 to c8c6648 Jun 21, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 21, 2017

The angular.io preview for c8c6648 is available here.

@jasonaden jasonaden force-pushed the jasonaden:feat_no_ng_zone branch from c8c6648 to 75bbe38 Jun 21, 2017

@jasonaden jasonaden requested a review from vicb Jun 21, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 21, 2017

The angular.io preview for 75bbe38 is available here.

@jasonaden jasonaden force-pushed the jasonaden:feat_no_ng_zone branch from 75bbe38 to 181e449 Jun 22, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 22, 2017

The angular.io preview for 181e449 is available here.

objs.reduce(optionsReducer, dst);
} else {
for (let key in objs) {
if (objs.hasOwnProperty(key)) {

This comment has been minimized.

Copy link
@vicb

vicb Jun 22, 2017

Contributor

Object.keys()

This comment has been minimized.

Copy link
@jasonaden

jasonaden Jun 22, 2017

Author Contributor

Spread operator.

@vicb

vicb approved these changes Jun 22, 2017

@jasonaden jasonaden force-pushed the jasonaden:feat_no_ng_zone branch from 181e449 to 8a13973 Jun 22, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 22, 2017

The angular.io preview for 8a13973 is available here.

expect(ngZone.onError instanceof EventEmitter).toBe(true);
expect(ngZone.onStable instanceof EventEmitter).toBe(true);
expect(ngZone.onUnstable instanceof EventEmitter).toBe(true);
expect(ngZone.onMicrotaskEmpty instanceof EventEmitter).toBe(true);

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 22, 2017

Member

doesn't typechecking verify this?

This comment has been minimized.

Copy link
@jasonaden

jasonaden Jun 23, 2017

Author Contributor

It verifies the type, but not the runtime instanceof check.

it('should run', () => {
let works = false;
ngZone.run(() => {
ngZone.runGuarded(() => { ngZone.runOutsideAngular(() => { works = true; }); });

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 22, 2017

Member

how does this proves that the NoNgZone works?

This comment has been minimized.

Copy link
@jasonaden

jasonaden Jun 23, 2017

Author Contributor

The test is for should run which this does prove since it's calling all the run* functions.

*
* @stable
*/
export class NoNgZone implements NgZone {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 22, 2017

Member

you described the zone as "noop", why do we then call it "No*"? I propose that you rename it to "NoopNgZone".

@@ -719,8 +733,8 @@ export declare const platformCore: (extraProviders?: Provider[] | undefined) =>
export declare abstract class PlatformRef {
readonly abstract destroyed: boolean;
readonly abstract injector: Injector;
/** @stable */ abstract bootstrapModule<M>(moduleType: Type<M>, compilerOptions?: CompilerOptions | CompilerOptions[]): Promise<NgModuleRef<M>>;
/** @experimental */ abstract bootstrapModuleFactory<M>(moduleFactory: NgModuleFactory<M>): Promise<NgModuleRef<M>>;
/** @stable */ abstract bootstrapModule<M>(moduleType: Type<M>, compilerOptions?: (CompilerOptions & BootstrapOptions) | Array<CompilerOptions & BootstrapOptions>): Promise<NgModuleRef<M>>;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 22, 2017

Member

BootstrapOptions are not being exported.

But more importantly, why do we export both the BootstrapOptions with "none" literal string API as well as No(op)NgZone. We should either not export No(op)NgZone, or export it and then allow BootstrapOptions to accept it without going through the "none" string.

And if we do stick to the "none" string literal, we should likely rename it to "noop" to be consistent with the internal naming as well as external behavior (it's not like NgZone doesn't exist, it's just that it doesn't do anything).

@jasonaden jasonaden force-pushed the jasonaden:feat_no_ng_zone branch from 8a13973 to 87ba8a5 Jun 23, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 23, 2017

The angular.io preview for 87ba8a5 is available here.

@jasonaden jasonaden force-pushed the jasonaden:feat_no_ng_zone branch from 87ba8a5 to 26e804f Jun 26, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 26, 2017

The angular.io preview for 26e804f is available here.

@@ -662,6 +662,20 @@ export declare class NgZone {
export declare const NO_ERRORS_SCHEMA: SchemaMetadata;

/** @stable */
export declare class NoNgZone implements NgZone {

This comment has been minimized.

Copy link
@mhevery

mhevery Jul 5, 2017

Member

NoNgZone is private implementation and should not be exported.

* - `default` - Use default `NgZone` which requires `Zone.js`.
* - `none` - Use `NoNgZone` which does nothing.
*/
ngZone?: NgZone|'default'|'none';

This comment has been minimized.

Copy link
@mhevery

mhevery Jul 5, 2017

Member

this should be NgZone|'zone.js'|'noop'

@@ -84,37 +84,58 @@ import {EventEmitter} from '../event_emitter';
* @experimental
*/
export class NgZone {

This comment has been minimized.

Copy link
@mhevery

mhevery Jul 5, 2017

Member
  • rename NgZone to ZoneJSNgZone.
  • Create new abstract class NgZone to be an injection token.
  • have ZoneJSNgZone and NoopNgZone extends NgZone

@mhevery mhevery force-pushed the angular:master branch from 08b36cb to 5d4b36f Jul 20, 2017

@mhevery mhevery force-pushed the jasonaden:feat_no_ng_zone branch from 26e804f to d1abfe7 Jul 26, 2017

@tbosch tbosch added the type: feature label Aug 3, 2017

@ocombe ocombe force-pushed the jasonaden:feat_no_ng_zone branch from d1abfe7 to d04cea8 Sep 13, 2017

@@ -383,7 +383,7 @@ export class UpgradeAdapter {
const windowAngular = (window as any /** TODO #???? */)['angular'];
windowAngular.resumeBootstrap = undefined;

this.ngZone.run(() => { angular.bootstrap(element, [this.ng1Module.name], config !); });
angular.bootstrap(element, [this.ng1Module.name], config !);

This comment has been minimized.

Copy link
@ocombe

ocombe Sep 13, 2017

Contributor

@IgorMinar @mhevery: I had to do this change to make the upgrade_spec tests pass, otherwise they would (almost) all fail with the error Error: Uncaught (in promise): Error: Expected to not be in Angular Zone, but it is!.
I don't know if it is the correct fix or not.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 13, 2017

Member

This doesn't look right 😟
(But no idea why the tests fail.)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 13, 2017

Member

AFAICT, this PR is just a refactoring as far as ngUpgrade is concerned. Under the hood, the exact same operations should happen while bootstrapping. It is surprizing that the behavior has changed.

(Do I miss something, @ocombe?)

This comment has been minimized.

Copy link
@ocombe

ocombe Sep 13, 2017

Contributor

I agree, I've searched in the updated code for what could be the reason for this, but I couldn't find it, that's why I added this comment saying that this change is probably incorrect

This comment has been minimized.

Copy link
@mhevery

mhevery Sep 13, 2017

Member

Yes, I don't see why the behavior should change here, and i would be hesitant to commit this in as is. We need to get to the bottom of this.

@matsko

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

@jasonaden @mhevery please rebase

@mhevery

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

Actually this is now owned by @ocombe and he said he will rebase.

@ocombe ocombe force-pushed the jasonaden:feat_no_ng_zone branch 2 times, most recently from e6f1c23 to 9165169 Sep 15, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 15, 2017

@ocombe ocombe force-pushed the jasonaden:feat_no_ng_zone branch from 9165169 to 0b64c26 Sep 15, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 15, 2017

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

can you please rebase?

@ocombe ocombe force-pushed the jasonaden:feat_no_ng_zone branch from 0b64c26 to c1a8391 Sep 21, 2017

@ocombe ocombe force-pushed the jasonaden:feat_no_ng_zone branch from c1a8391 to 64256bd Sep 21, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 21, 2017

@ocombe

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

Rebased!

@googlebot googlebot added cla: no and removed cla: yes labels Sep 21, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 21, 2017

@IgorMinar IgorMinar added cla: yes and removed cla: no labels Sep 21, 2017

@IgorMinar IgorMinar closed this in 344a5ca Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.