Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upfix(platform-browser): simple version of zone aware addEventListener #18993
Conversation
googlebot
added
the
cla: yes
label
Sep 1, 2017
JiaLiPassion
force-pushed the
JiaLiPassion:event
branch
from
8141be5
to
4ab2c5b
Sep 1, 2017
JiaLiPassion
force-pushed the
JiaLiPassion:event
branch
from
4ab2c5b
to
6064d7b
Sep 1, 2017
JiaLiPassion
referenced this pull request
Sep 1, 2017
Closed
DomEventPlugin forces event handlers to run in Angular Zone #18753
mhevery
requested changes
Sep 1, 2017
| if (!blackListedEvents) { | ||
| return false; | ||
| } | ||
| return blackListedEvents.filter(blackListedEvent => blackListedEvent === eventName).length > 0; |
This comment has been minimized.
This comment has been minimized.
mhevery
Sep 1, 2017
Member
Could we turn the events into map. Calling filter all the time has negative perf implication. This would be faster. blackListMap.hasOwnProperty(eventName)
This comment has been minimized.
This comment has been minimized.
| }; | ||
| // if zonejs is loaded and current zone is not ngZone | ||
| // we keep Zone.current on target for later restoration. | ||
| const isBlackListed = isBlackListedEvent(eventName); |
This comment has been minimized.
This comment has been minimized.
mhevery
Sep 1, 2017
Member
PERF: We should delay checking if it is black-listed until after we know we have Zone.
This comment has been minimized.
This comment has been minimized.
JiaLiPassion
Sep 2, 2017
Author
Contributor
@mhevery , ok, I will check it after zoneJsLoaded is true.
JiaLiPassion
force-pushed the
JiaLiPassion:event
branch
from
8bd5352
to
eaab36c
Sep 2, 2017
This comment has been minimized.
This comment has been minimized.
|
@mhevery , I have updated the commits.
please review. |
mhevery
requested changes
Sep 2, 2017
| const NATIVE_ADD_LISTENER = 'addEventListener'; | ||
| const NATIVE_REMOVE_LISTENER = 'removeEventListener'; | ||
|
|
||
| const blackListedMap: {[key: string]: string} = Zone && Zone[__symbol__('BLACK_LISTED_EVENTS')]; |
This comment has been minimized.
This comment has been minimized.
mhevery
Sep 2, 2017
Member
Sorry I meant that you would convert the array into the map, rather than expect that the data is provided as a map.
This comment has been minimized.
This comment has been minimized.
JiaLiPassion
Sep 2, 2017
•
Author
Contributor
@mhevery , got it, sorry for misunderstanding, I have updated the source, please review.
JiaLiPassion
force-pushed the
JiaLiPassion:event
branch
from
eaab36c
to
6c01b2c
Sep 2, 2017
This comment has been minimized.
This comment has been minimized.
|
@mhevery , I have updated the source and write a sample code to describe how to set Please review. |
mhevery
approved these changes
Sep 2, 2017
mhevery
added
PR action: merge
PR target: master-only
labels
Sep 2, 2017
This comment has been minimized.
This comment has been minimized.
|
@mhevery , I found a bug which is the same one with angular/zone.js#839, the issue can be described as the following case. const handler1 = (e: any)=> {
// handler remove itself
remover1 && remover1();
};
const handler2 = (e: any) => {
console.log('handler2');
};
const manager = new EventManager([domEventPlugin], new FakeNgZone());
remover1 = manager.addEventListener(element, 'click', handler1);
remover2 = manager.addEventListener(element, 'click', handler2);
getDOM().dispatchEvent(element, dispatchedEvent);in previous version, The modified code in // a global listener to handle all dom event,
// so we do not need to create a closure everytime
const globalListener = function(event: Event) {
const symbolName = symbolNames[event.type];
if (!symbolName) {
return;
}
const taskDatas: TaskData[] = this[symbolName];
if (!taskDatas) {
return;
}
const args: any = [event];
if (taskDatas.length === 1) {
// if taskDatas only have one element, just invoke it
const taskData = taskDatas[0];
if (taskData.zone !== Zone.current) {
// only use Zone.run when Zone.current not equals to stored zone
return taskData.zone.run(taskData.handler, this, args);
} else {
return taskData.handler.apply(this, args);
}
} else {
// copy tasks as a snapshot to avoid event handlers remove
// itself or others
const copiedTasks = taskDatas.slice();
for (let i = 0; i < copiedTasks.length; i++) {
const taskData = copiedTasks[i];
if (taskData.zone !== Zone.current) {
// only use Zone.run when Zone.current not equals to stored zone
taskData.zone.run(taskData.handler, this, args);
} else {
taskData.handler.apply(this, args);
}
}
}
};
it is the same with angular/zone.js#839 , please review. |
JiaLiPassion
force-pushed the
JiaLiPassion:event
branch
from
6c01b2c
to
7ed0c53
Sep 5, 2017
JiaLiPassion
force-pushed the
JiaLiPassion:event
branch
from
7ed0c53
to
5bcd96e
Sep 5, 2017
mhevery
closed this
in
ed1175f
Sep 6, 2017
added a commit
that referenced
this pull request
Sep 6, 2017
This comment has been minimized.
This comment has been minimized.
PierreDuc
commented
Sep 15, 2017
|
Little late to the party, but doesn't the introduction of a |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
PierreDuc
commented
Sep 15, 2017
•
|
@JiaLiPassion I guess sample code will be fine: Let's say your
And your
Then even though you call |
This comment has been minimized.
This comment has been minimized.
|
@PierreDuc , got it, thank you, I can fix this one. |


JiaLiPassion commentedSep 1, 2017
•
edited
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: 18753
What is the new behavior?
EventHandler will run into the correct zone which will be the zone when
addEventListeneris invokedDoes this PR introduce a breaking change?
Other information
fix #18753.
Add a simple version of zone aware addEventListener.
globalEventHandlerto handle all events to avoid creating to many closure function.zonewhenaddEventListener, but for better performance, we don't usezone.jsmechanism toscheduleEventTask, soZoneSpec.onInvokeTask,ZoneSpec.onScheduleTask,ZoneSpec.onCancelTaskwill not be invoked.addEventListener, if theZone.currentisngZone, we just usenative addEventListenerfor better performance (which will be the most cases).blackListedEventsso user can decide which events they want to run outside of ngZone.Here is a sample to run all
scrollevent handler outside of ngZone.@mhevery , please review.