-
Notifications
You must be signed in to change notification settings - Fork 407
fix(webapi): refactor webapi to not import util.ts directly #652
Conversation
JiaLiPassion
commented
Feb 25, 2017
- webapi mediaquery, notification should not import methods from util.ts because it will pull all util.ts content into webapi-media-query.js and webapis-notification.js under dist
- export some zone util's method through Zone[symbol(methodName)]
- add webapis dist/min.js
b55b4ad
to
f3b8885
Compare
ac951ee
to
d416671
Compare
@@ -23,16 +22,14 @@ import {NestedEventListenerOrEventListenerObject, patchEventTargetMethods} from | |||
handler: args[0], | |||
target: self || _global, | |||
name: 'mediaQuery', | |||
invokeAddFunc: function( | |||
addFnSymbol: any, delegate: Task|NestedEventListenerOrEventListenerObject) { | |||
invokeAddFunc: function(addFnSymbol: any, delegate) { |
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.
Why are you removing the types? That does not seem right.
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.
The reason is I don't want to import the type from until.ts which will drag all contents from util.ts when rollup build. Just like the review here,#631 (review)
@@ -569,3 +569,15 @@ export function findEventTask(target: any, evtName: string): Task[] { | |||
} | |||
return result; | |||
} | |||
|
|||
Zone[zoneSymbol('patchEventTargetMethods')] = patchEventTargetMethods; |
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 feel very uneasy with exposing these APIs even if they are behind a symbol. What is the reasoning?
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.
Yes,I feel the same,the reason to expose those API in such way is
- for easier patch other non standard or 3rd party API
- don't need to import those methods from util.ts
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 think we should expose the minimal set. I will remove the rest of the exports and just keep patchEventTargetMethods
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.
sure, mediaQuery use patchEventTargetMethods, and Notification use patchOnProperties. I will leave those two and remove others.