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

add generic patch eventtarget method#549

Merged
mhevery merged 3 commits intoangular:masterfrom
JiaLiPassion:general-eventtask
Dec 18, 2016
Merged

add generic patch eventtarget method#549
mhevery merged 3 commits intoangular:masterfrom
JiaLiPassion:general-eventtask

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Collaborator

@JiaLiPassion JiaLiPassion commented Dec 17, 2016 •
edited
Loading

now util.ts provide a patchEventTargetMethods to patch dom addEventListener and nodejs EventEmitter addEventListener method.
becasue dom/node addEventListener has the similar signature.

   addEventListener(eventName, handler, ...);

It is ok for dom element and node, but in other case, for example in PR #543, if we want to patch
other eventTarget like MediaQuery , the add listener method signature is different

addListener(MediaQueryListListener listener);

So if we want to patch it, we need to write another patch logic like #543, it is not effective and in the future, if we want to patch other event Target like notification API like #536, we will need another same logic, so we need a generic way to patch event target like object.

In this PR, I provide a ListenerTaskMeta creator to create related information for customize, so
to fix #543, we may call like this.

patchEventTargetMethods(MediaQuerlList.prototype, 'addListener', 'removeListener', (self, args) => {
    return {
     useCapturing: false,
     eventName: 'mediaQuery',
     handler: args[0],
     target: self || _global,
     name: 'mediaQuery',
     invokeAddFunc: function (addFnSymbol: any, delegate: Task | NestedEventListenerOrEventListenerObject) {
         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) {
       if (delegate && (<Task>delegate).invoke) {
         return this.target[removeFnSymbol]((<Task>delegate).invoke);
       } else {
         return this.target[removeFnSymbol](delegate);
       }
     }
   };
})

@mhevery mhevery merged commit 0fd8c03 into angular:master Dec 18, 2016
@poxrud
Copy link
Copy Markdown
Contributor

poxrud commented Dec 18, 2016

@JiaLiPassion please add the PR for MatchMedia support using your new patchEventTargetMethods. You can use the tests in my branch to validate.

@JiaLiPassion
Copy link
Copy Markdown
Collaborator Author

JiaLiPassion commented Dec 19, 2016

@poxrud , ok, thank you, I will make a PR with the generic method and your test code.

@JiaLiPassion JiaLiPassion deleted the general-eventtask branch December 19, 2016 00:27
@JiaLiPassion
Copy link
Copy Markdown
Collaborator Author

JiaLiPassion commented Dec 20, 2016

@poxrud , I am testing the code in my branch now, the problem is that a lot of different errors occur in the test codes on IE10/IE11/Safari9/Android, I add some code to check the MediaQuery availability, but still error occurs, it will take some time for me to fix them, and I think the MediaQuery is so experimental feature, maybe at last you should call a API (zone.patchMediaQuery) from app side. I will continue to debug the test code in those env (IE/Android)

@poxrud
Copy link
Copy Markdown
Contributor

poxrud commented Dec 20, 2016

Yes you need to check if MatchMedia is supported with something like:

if (_global['MediaQueryList']) {
 patchEventTargetMethods...
}

If you don't mind pasting your patching code I'd like to test it out myself as well.

@JiaLiPassion
Copy link
Copy Markdown
Collaborator Author

JiaLiPassion commented Dec 20, 2016

@poxrud , yeah, I have added the detection code, and the patch part is ok, but the test code will throw a lot of error, you can find my commits here, commits, and you can check Travis log to see the error details. the code is on my branch issue-543.

@JiaLiPassion JiaLiPassion mentioned this pull request Dec 31, 2016
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JiaLiPassion @poxrud @mhevery @googlebot