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

fix(task): fix #832, fix #835, task.data should be an object #834

Merged
merged 3 commits into from
Jul 14, 2017

Conversation

JiaLiPassion
Copy link
Collaborator

@JiaLiPassion JiaLiPassion commented Jul 13, 2017

fix #832.
fix #835.

  • Task.data should not be a string but an object.
  • should define _global.

@@ -24,6 +29,9 @@ export const ZONE_SYMBOL_PREFIX = '__zone_symbol__';

const EVENT_NAME_SYMBOL_REGX = /^__zone_symbol__(\w+)(true|false)$/;

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 + ')\$');

Copy link
Collaborator Author

@JiaLiPassion JiaLiPassion Jul 13, 2017

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.

Choose a reason for hiding this comment

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

agreed.

Copy link

@corvinrok corvinrok 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 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

@JiaLiPassion
Copy link
Collaborator Author

@kgentes , thank you for review, the code is a little difficult to understand than the earlier version because of the performance concern.

@@ -24,6 +29,9 @@ export const ZONE_SYMBOL_PREFIX = '__zone_symbol__';

const EVENT_NAME_SYMBOL_REGX = /^__zone_symbol__(\w+)(true|false)$/;

const _global =
Copy link
Contributor

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.

Copy link
Collaborator Author

@JiaLiPassion JiaLiPassion Jul 13, 2017

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.

@mhevery mhevery merged commit 3a4bfbd into angular:master Jul 14, 2017
@JiaLiPassion JiaLiPassion deleted the issue-832 branch July 14, 2017 03:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants