Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix type of CustomEvent detail attribute #271

Merged
merged 1 commit into from Mar 1, 2017

Conversation

andersschuller
Copy link
Contributor

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

When addressing this review, amend your commit, please. Do not create a second commit.

@@ -5091,7 +5091,7 @@ class CustomEvent extends Event {
*
* MDN
*/
def detail: Object = js.native
def detail: js.Any = js.native
Copy link
Member

Choose a reason for hiding this comment

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

It should be Any then, not js.Any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. I guess I assumed it should be js.Any since it seemed to be consistent with most other similar attributes/methods. For example, the History API has the following:

def state: js.Any = js.native
def pushState(statedata: js.Any, title: String): Unit = js.native

There only seem to be a handful of uses of java.lang.Object, but now that I look more closely, there are quite a few uses of scala.Any. There are even a few cases that are not consistent within the same class, for example MessageEvent. This uses scala.Any for the data attribute but js.Any for the corresponding argument to initMessageEvent.

I'm not sure I understand why all these would be slighly different. Is it just an artifact of having imported them from the TypeScript type definitions? I'm happy to make the change you requested, of course, just thought I would clarify first.

Copy link
Member

Choose a reason for hiding this comment

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

Most js.Anys are artifacts of importing from TypeScript, and should be Any.

@@ -5100,7 +5100,7 @@ class CustomEvent extends Event {
* MDN
*/
def initCustomEvent(typeArg: String, canBubbleArg: Boolean,
cancelableArg: Boolean, detailArg: Object): Unit = js.native
cancelableArg: Boolean, detailArg: js.Any): Unit = js.native
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@andersschuller
Copy link
Contributor Author

OK, commit amended with the requested changes.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thanks.

@sjrd sjrd merged commit e244bb3 into scala-js:master Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants