-
Notifications
You must be signed in to change notification settings - Fork 408
fix(task): fix #832, fix #835, task.data should be an object #834
Conversation
lib/common/events.ts
Outdated
@@ -24,6 +29,9 @@ export const ZONE_SYMBOL_PREFIX = '__zone_symbol__'; | |||
|
|||
const EVENT_NAME_SYMBOL_REGX = /^__zone_symbol__(\w+)(true|false)$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you are already creating TRUE_STR and FALSE_STR on line 10 and line 11, why not reuse those consts here by generating the regex with something like
let evt_name_sym_regx = new RegExp('^__zone_symbol__(\\w+)(' +TRUE_STR + '|' + FALSE_STR + ')\$');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgentes , yes, you are right, I just checked again, because this regx is just initialized once, so I think it is ok to not reuse TRUE_STR and FALSE_STR. It will be more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still learning this code, so I don't feel confident enough to give an official approval. However, I did review this change across the three files submitted, and do agree with the changes related to fix #834
@kgentes , thank you for review, the code is a little difficult to understand than the earlier version because of the performance concern. |
lib/common/events.ts
Outdated
@@ -24,6 +29,9 @@ export const ZONE_SYMBOL_PREFIX = '__zone_symbol__'; | |||
|
|||
const EVENT_NAME_SYMBOL_REGX = /^__zone_symbol__(\w+)(true|false)$/; | |||
|
|||
const _global = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get global
from __load_patch
? I would prefer to have the global
logic centralized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhevery , I have changed the structure and let patchEventTargetMethods to use the _global
from __load_patch, and travis CI has passed, please review.
fix #832.
fix #835.