Skip to content

Update EventTarget according to the latest spec#1959

Closed
vobruba-martin wants to merge 1 commit intogoogle:masterfrom
vobruba-martin:eventtarget-fix
Closed

Update EventTarget according to the latest spec#1959
vobruba-martin wants to merge 1 commit intogoogle:masterfrom
vobruba-martin:eventtarget-fix

Conversation

@vobruba-martin
Copy link
Copy Markdown
Contributor

@vobruba-martin vobruba-martin commented Aug 17, 2016 •
edited
Loading

EventTarget is currently defined according to DOM Level 2 spec and this is causing some problems:

  1. You cannot pass options object when creating event listeners which is needed if you want to create passive or one time event listeners.
window.addEventListener('click', function() {}, { capture:false, passive:true, once:true });
JSC_TYPE_MISMATCH: actual parameter 3 of Window.prototype.addEventListener does not match formal parameter
found   : {
  capture: boolean,
  once: boolean,
  passive: boolean
}
required: (boolean|undefined) at line 1 character 48
window.addEventListener('click', function() {}, { capture:false, passive:true...
                                                ^
  1. All 3 arguments are required on interfaces which extend EventTarget but do not override its methods. For example WorkerGlobalScope:
/** @type {!WorkerGlobalScope} */
var x;
x.addEventListener('click', function() {});
JSC_WRONG_ARGUMENT_COUNT: Function EventTarget.addEventListener: called with 2 argument(s). Function requires at least 3 argument(s) and no more than 3 argument(s). at line 3 character 0
x.addEventListener('click', function() {});
^

This PR updates EventTarget according to the latest spec.

@aghassemi
Copy link
Copy Markdown

aghassemi commented Sep 19, 2016

@MatrixFrog @Dominator008 What's the likelihood of this PR getting accepted? We are running into the same issue where we like to use the new Event API and considering we can't override builtin externs in our custom extern with a different signature, we are kind of stuck.

@MatrixFrog
Copy link
Copy Markdown
Contributor

MatrixFrog commented Sep 19, 2016

Sounds good based on a quick glance. I can probably review it this week if someone else doesn't get to it first.

@aghassemi aghassemi mentioned this pull request Sep 19, 2016
Closed
@MatrixFrog MatrixFrog self-assigned this Sep 26, 2016
@MatrixFrog
Copy link
Copy Markdown
Contributor

MatrixFrog commented Sep 28, 2016

This turns out to be harder to land than I thought. Every subclass of EventTarget has to be updated if it has the @param annotations written out instead of just @override. You've fixed a bunch of them in this PR but there are at least a few others within Google's codebase. Hopefully I can get them all updated in the next few days, and get this submitted.

@vobruba-martin
Copy link
Copy Markdown
Contributor Author

vobruba-martin commented Sep 29, 2016

Thank you!

@MatrixFrog
Copy link
Copy Markdown
Contributor

MatrixFrog commented Oct 6, 2016

Getting close.

@vobruba-martin
Copy link
Copy Markdown
Contributor Author

vobruba-martin commented Nov 8, 2016

Rebased because of this commit 3b23f1e

@MatrixFrog Do you have any ETA on this?

@vobruba-martin
Copy link
Copy Markdown
Contributor Author

vobruba-martin commented Jan 13, 2017

Rebased

@MatrixFrog
Copy link
Copy Markdown
Contributor

MatrixFrog commented Jan 13, 2017

Waiting on a response from an internal team, so unfortunately I do not.

@vobruba-martin vobruba-martin mentioned this pull request Jan 15, 2017
Closed
@vobruba-martin
Copy link
Copy Markdown
Contributor Author

vobruba-martin commented Feb 24, 2017

@MatrixFrog Any update? :-)

@MatrixFrog
Copy link
Copy Markdown
Contributor

MatrixFrog commented Mar 3, 2017

I'm going to really really try to get this in by next week.

@MatrixFrog
Copy link
Copy Markdown
Contributor

MatrixFrog commented Mar 9, 2017

This is landed internally. However, don't get excited yet; we might need to roll it back for some reason.

@dimvar dimvar closed this in 8ac08c0 Mar 9, 2017
concavelenz pushed a commit to google/closure-library that referenced this pull request Mar 9, 2017
@concavelenz
Update EventTarget according to the latest spec. Requires changes in …
8cc4b59 
…Closure Library as well, because it is no longer possible for a class to implement the EventTarget interface while also extending the goog.events.EventTarget class.

Closes google/closure-compiler#1959

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149657444
@aghassemi aghassemi mentioned this pull request Aug 1, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@MatrixFrog MatrixFrog

Labels

cla: yes Externs Import In Progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vobruba-martin @aghassemi @MatrixFrog @Dominator008 @googlebot