Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

add Zone.root to get root zone directly #601

Merged
mhevery merged 1 commit intoangular:masterfrom
JiaLiPassion:outside
Feb 7, 2017
Merged

add Zone.root to get root zone directly #601
mhevery merged 1 commit intoangular:masterfrom
JiaLiPassion:outside

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Collaborator

@JiaLiPassion JiaLiPassion commented Jan 15, 2017 •
edited
Loading

add runOutsideOfCurrentZone method just like runOutsideAngular so we can run under rootZone or fork a new zone under rootZone.

Zone.current.fork({name: 'testZone'}).run(function() {
        Zone.runOutsideOfCurrentZone(() => {
          expect(Zone.current.name).toEqual('<root>');
        });
      });

The motivation is to run some async code inside the zoneSpec's callback. Without the runOutsideOfCurrentZone method, such async call will cause infinite loop.
for example:

Zone.current.fork({
  name: 'testZone', 
  onScheduleTask: function(delegate, currentZone, targetZone, task) {
     asyncLog(task);
     return delegate.scheduleTask(targetZone, task);
  }
});

asyncLog will cause infinite loop, now we can

Zone.current.fork({
  name: 'testZone', 
  onScheduleTask: function(delegate, currentZone, targetZone, task) {
     Zone.runOutsideCurrentZone(() => {
        asyncLog(task);
    });
     return delegate.scheduleTask(targetZone, task);
  }
});

@xtianjohns xtianjohns mentioned this pull request Jan 16, 2017
Closed
mhevery
mhevery reviewed Jan 23, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

I am sorry, but I don't see the need for this API. I am fine with adding root property, but the rest seems just sugar.

My concern is that new APIs need to be maintained, and documented. Personally my philosophy is to just give the bare minimum and have the developer build on top of that.

Comment thread lib/zone.ts Outdated
}
}

static get rootZone(): AmbientZone {
Copy link
Copy Markdown
Contributor

@mhevery mhevery Jan 23, 2017

Choose a reason for hiding this comment

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

I am fine with this API, but I would rename it to root

Comment thread lib/zone.ts Outdated
return zone;
}

static runOutsideOfCurrentZone(
Copy link
Copy Markdown
Contributor

@mhevery mhevery Jan 23, 2017

Choose a reason for hiding this comment

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

I think this API gives too much complexity to the user. Same can be achieved by:

Zone.root.fork(zoneSpec).run(....)

So I don't see the need to have it.

@JiaLiPassion
Copy link
Copy Markdown
Collaborator Author

JiaLiPassion commented Jan 23, 2017 •
edited
Loading

@mhevery , Ok, got it,I have modified the PR, please review

@JiaLiPassion JiaLiPassion changed the title add runOutsideOfCurrentZone just like runOutsideOfAngular add Zone.root to get root zone directly Jan 24, 2017
@mhevery mhevery merged commit 9818139 into angular:master Feb 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mhevery mhevery mhevery left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JiaLiPassion @mhevery @googlebot