Skip to content

Fix #219: Fix Event constructors. #367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
May 16, 2019
Merged

Fix #219: Fix Event constructors. #367

merged 11 commits into from
May 16, 2019

Conversation

exoego
Copy link
Contributor

@exoego exoego commented May 15, 2019 •
edited
Loading

Closes #219

  • Add valid constructors that accept type name and init, as well as type-safe constructor options ***EventInit
  • Removes invalid default (0 args) constructors since new ***Event() raises RuntimeError due to lack of required argument
  • Add @deprecated based on MDN

@exoego exoego force-pushed the event-constructor branch from 68e265a to 1c90901 Compare May 15, 2019 14:03
sjrd
sjrd requested changes May 15, 2019
View reviewed changes
src/main/scala/org/scalajs/dom/experimental/deviceorientation/DeviceOrientation.scala Outdated
@@ -29,23 +28,23 @@ class DeviceOrientationEvent(
val absolute: Boolean = js.native
}

trait DeviceOrientationEventInit extends js.Object {
trait DeviceOrientationEventInit extends dom.raw.EventInit {
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend importing dom.raw.EventInit at the top of the file, and then use EventInit everywhere. There might be other files where this comment applies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 882c079

src/main/scala/org/scalajs/dom/experimental/serviceworkers/ServiceWorkers.scala Outdated
var isReload: js.UndefOr[Boolean] = js.undefined
var request: js.UndefOr[Request] = js.undefined
var clientId: js.UndefOr[String] = js.undefined

Copy link
Member

Choose a reason for hiding this comment

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

And there should be no blank line here.

src/main/scala/org/scalajs/dom/experimental/serviceworkers/ServiceWorkers.scala Outdated

/**
* Returns the event's data. It can be any data type.
*/
val data: Any = js.native
val data: 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.

Why did this become a js.Any? Is the data interpreted by JS code in any way? If it is just carried around, it should stay Any.

src/main/scala/org/scalajs/dom/raw/lib.scala Outdated
@@ -3538,12 +3579,13 @@ class MessageEvent extends Event {
*
* MDN
*/
def data: Any = js.native
def data: String | Blob | ArrayBuffer = 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.

MDN says that data can be any data type, so this should stay 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.

Fixed in 4457811

src/main/scala/org/scalajs/dom/raw/lib.scala Outdated

@deprecated("Non-standard", "forever")
def initMessageEvent(typeArg: String, canBubbleArg: Boolean,
cancelableArg: Boolean, dataArg: js.Any, originArg: String,
lastEventIdArg: String, sourceArg: Window): Unit = js.native
cancelableArg: Boolean, dataArg: String | Blob | ArrayBuffer,
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be Any as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4457811

src/main/scala/org/scalajs/dom/raw/lib.scala Outdated
@@ -5648,14 +5718,19 @@ class StyleSheetList extends js.Object {
def update(index: Int, v: StyleSheet): Unit = js.native
}

trait CustomEventInit extends EventInit {
var detailArg: js.UndefOr[js.Any] = js.undefined
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure this should an Any as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4457811

src/main/scala/org/scalajs/dom/raw/lib.scala Outdated
@@ -5851,10 +5927,17 @@ class TimeRanges extends js.Object {

@js.native
@JSGlobal
class BeforeUnloadEvent extends Event {
class BeforeUnloadEvent extends Event("") {
Copy link
Member

Choose a reason for hiding this comment

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

It is more customary to use js.native as the arguments to another argument, in order to be confused that "" is not actually taken into account.

Copy link
Contributor Author

@exoego exoego May 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I did not know the custom / tips.
That sounds reasonable.
Fixed in b7194a5

src/main/scala/org/scalajs/dom/raw/lib.scala Outdated
@@ -4829,8 +4829,7 @@ trait MutationEventInit extends EventInit {
"WHATWG DOM")
@js.native
@JSGlobal
class MutationEvent(typeArg: String, init: js.UndefOr[MutationEventInit])
extends Event(typeArg, init) {
class MutationEvent extends Event("", js.undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

If it has no constructor, its constructor should be marked private. Otherwise there is nothing preventing to do new MutationEvent from the Scala side (with a run-time error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 879d4ae

Used private[this] instead of private(), thanks to deprecation warning declaring private constructors in native JS classes is deprecated, because they do not behave the same way as in Scala.js-defined JS classes. Use `private[this]` instead. This will become an error in 1.0.0.

@exoego exoego mentioned this pull request May 16, 2019
Closed
sjrd
sjrd reviewed May 16, 2019
View reviewed changes
src/main/scala/org/scalajs/dom/raw/lib.scala Outdated
@@ -4772,10 +4822,15 @@ abstract class DocumentType extends Node {
def publicId: String = js.native
}

@deprecated("Obsolete.", "WHATWG DOM")
trait MutationEventInit extends EventInit {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this definition? Couldn't we just not define it at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 96938be

@sjrd sjrd changed the title Fix Event constructors Fix Event constructors May 16, 2019
@sjrd sjrd changed the title Fix Event constructors Fix #219: Fix Event constructors. May 16, 2019
sjrd
sjrd approved these changes May 16, 2019
View reviewed 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.

Thank you!

@sjrd sjrd merged commit 159526a into scala-js:master May 16, 2019
@exoego exoego deleted the event-constructor branch May 16, 2019 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invalid Event constructor
2 participants
@exoego @sjrd