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

fix(webapi): refactor webapi to not import util.ts directly #652

Closed
wants to merge 1 commit into from
Closed

fix(webapi): refactor webapi to not import util.ts directly #652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,19 @@ gulp.task('build/zone.min.js', ['compile-esm'], function(cb) {
});

gulp.task('build/webapis-media-query.js', ['compile-esm'], function(cb) {
return generateScript('./lib/browser/webapis-media-query.ts', 'webapis-media-query.js', true, cb);
return generateScript('./lib/browser/webapis-media-query.ts', 'webapis-media-query.js', false, cb);
});

gulp.task('build/webapis-media-query.min.js', ['compile-esm'], function(cb) {
return generateScript('./lib/browser/webapis-media-query.ts', 'webapis-media-query.min.js', true, cb);
});

gulp.task('build/webapis-notification.js', ['compile-esm'], function(cb) {
return generateScript('./lib/browser/webapis-notification.ts', 'webapis-notification.js', true, cb);
return generateScript('./lib/browser/webapis-notification.ts', 'webapis-notification.js', false, cb);
});

gulp.task('build/webapis-notification.min.js', ['compile-esm'], function(cb) {
return generateScript('./lib/browser/webapis-notification.ts', 'webapis-notification.min.js', true, cb);
});

gulp.task('build/bluebird.js', ['compile-esm'], function(cb) {
Expand Down Expand Up @@ -173,7 +181,9 @@ gulp.task('build', [
'build/zone.min.js',
'build/zone-node.js',
'build/webapis-media-query.js',
'build/webapis-media-query.min.js',
'build/webapis-notification.js',
'build/webapis-notification.min.js',
'build/zone-mix.js',
'build/bluebird.js',
'build/bluebird.min.js',
Expand Down
9 changes: 3 additions & 6 deletions lib/browser/webapis-media-query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {NestedEventListenerOrEventListenerObject, patchEventTargetMethods} from '../common/utils';

((_global: any) => {
// patch MediaQuery
patchMediaQuery(_global);
Expand All @@ -15,6 +13,7 @@ import {NestedEventListenerOrEventListenerObject, patchEventTargetMethods} from
if (!_global['MediaQueryList']) {
return;
}
const patchEventTargetMethods = Zone[Zone['__symbol__']('patchEventTargetMethods')];
patchEventTargetMethods(
_global['MediaQueryList'].prototype, 'addListener', 'removeListener', (self, args) => {
return {
Expand All @@ -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) {
Copy link
Contributor

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.

Copy link
Collaborator Author

@JiaLiPassion JiaLiPassion Mar 8, 2017 •
edited
Loading

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)

if (delegate && (<Task>delegate).invoke) {
return this.target[addFnSymbol]((<Task>delegate).invoke);
} else {
return this.target[addFnSymbol](delegate);
}
},
invokeRemoveFunc: function(
removeFnSymbol: any, delegate: Task|NestedEventListenerOrEventListenerObject) {
invokeRemoveFunc: function(removeFnSymbol: any, delegate) {
if (delegate && (<Task>delegate).invoke) {
return this.target[removeFnSymbol]((<Task>delegate).invoke);
} else {
Expand Down
8 changes: 5 additions & 3 deletions lib/browser/webapis-notification.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {patchOnProperties} from '../common/utils';

((_global: any) => {
// patch Notification
patchNotification(_global);
Expand All @@ -16,7 +14,11 @@ import {patchOnProperties} from '../common/utils';
if (!Notification || !Notification.prototype) {
return;
}

const desc = Object.getOwnPropertyDescriptor(Notification.prototype, 'onerror');
if (!desc || !desc.configurable) {
return;
}
const patchOnProperties = Zone[Zone['__symbol__']('patchOnProperties')];
patchOnProperties(Notification.prototype, null);
}
})(typeof window === 'object' && window || typeof self === 'object' && self || global);
12 changes: 12 additions & 0 deletions lib/common/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,3 +569,15 @@ export function findEventTask(target: any, evtName: string): Task[] {
}
return result;
}

Zone[zoneSymbol('patchEventTargetMethods')] = patchEventTargetMethods;
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Zone[zoneSymbol('patchMicroTask')] = patchMicroTask;
Zone[zoneSymbol('patchMacroTask')] = patchMacroTask;
Zone[zoneSymbol('patchClass')] = patchClass;
Zone[zoneSymbol('patchProperty')] = patchProperty;
Zone[zoneSymbol('patchOnProperties')] = patchOnProperties;
Zone[zoneSymbol('isNode')] = isNode;
Zone[zoneSymbol('isBrowser')] = isBrowser;
Zone[zoneSymbol('isMix')] = isMix;
Zone[zoneSymbol('isWebWorker')] = isWebWorker;
Zone[zoneSymbol('findEventTask')] = findEventTask;
2 changes: 1 addition & 1 deletion test/browser/MediaQuery.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe('test mediaQuery patch', ifEnvSupports(supportMediaQuery, () => {
it('test whether addListener is patched', () => {
const mqList = window.matchMedia('min-width:500px');
if (mqList && mqList['addListener']) {
expect(mqList[zoneSymbol('addListener')]).not.toBe(undefined);
expect(mqList[zoneSymbol('addListener')]).toBeTruthy();
}
});
}));
27 changes: 27 additions & 0 deletions test/browser/Notification.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import '../../lib/browser/webapis-notification';

import {zoneSymbol} from '../../lib/common/utils';
import {ifEnvSupports} from '../test-util';

function notificationSupport() {
const desc = window['Notification'] &&
Object.getOwnPropertyDescriptor(window['Notification'].prototype, 'onerror');
return window['Notification'] && window['Notification'].prototype && desc.configurable;
}

(<any>notificationSupport).message = 'Notification Support';

describe('Notification API', ifEnvSupports(notificationSupport, function() {
it('Notification API should be patched by Zone', () => {
const Notification = window['Notification'];
expect(Notification.prototype[zoneSymbol('addEventListener')]).toBeTruthy();
});
}));
1 change: 1 addition & 0 deletions test/browser_entry_point.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ import './browser/requestAnimationFrame.spec';
import './browser/WebSocket.spec';
import './browser/XMLHttpRequest.spec';
import './browser/MediaQuery.spec';
import './browser/Notification.spec';
import './mocha-patch.spec';
import './jasmine-patch.spec';