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

Performance api improvements #709

Merged
merged 9 commits into from
Sep 27, 2023
Merged

Performance api improvements #709

merged 9 commits into from
Sep 27, 2023

Conversation

cquiroz
Copy link
Contributor

@cquiroz cquiroz commented Jul 4, 2022

These are some improvements to the Performance API after testing it for a while

@@ -15,7 +15,7 @@ import scala.scalajs.js.annotation._
*/
@js.native
@JSGlobal
class Performance extends js.Object {
class Performance private[this] () extends js.Object {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cannot create objects of type Performance. Is this the correct way to forbid it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think we do this in other places as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is correct.

@@ -15,7 +15,7 @@ import scala.scalajs.js.annotation._
*/
@js.native
@JSGlobal
class Performance extends js.Object {
class Performance private[this] () extends js.Object {
Copy link
Member

Choose a reason for hiding this comment

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

Yes I think we do this in other places as well.

@@ -28,7 +28,7 @@ class Performance extends js.Object {
*/
def timing: PerformanceTiming = js.native

def getEntriesByType(entryType: String): js.Dynamic = js.native
def getEntriesByType(entryType: String): js.Array[PerformanceEntry] = 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.

Since there are only a limited set of values that can be passed as an entryType I wonder if we should create a specific type for it.
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry/entryType

E.g. like InputType.
https://github.com/scala-js/scala-js-dom/blob/1a944e4863aaf52694e29e38fb2a174f351f2ceb/dom/src/main/scala-3/org/scalajs/dom/InputType.scala

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe I was wrong 😅

As the list of supported entries varies per browser and is evolving, this property allows web developers to check which are available.

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceObserver/supportedEntryTypes

Comment on lines +22 to 25
/** The first timestamp recorded for this performance entry. The meaning of this property depends on the value of this
* entry's [[entryType]].
*/
def startTime: Double = js.native
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec mentions that this returns a "DOMHighResTimestamp" https://w3c.github.io/performance-timeline/#dom-performanceentry

Is it worth it to make a type for https://www.w3.org/TR/hr-time-3/#dom-domhighrestimestamp

Copy link
Member

@armanbilge armanbilge Sep 27, 2023

Choose a reason for hiding this comment

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

We probably don't want an opaque type, since then you can't do numerical ops on it.

We could make a transparent type alias e.g. type DOMHighRestTimestamp = Double but it's a coin flip whether its documentation or obfuscation 😂

armanbilge and others added 2 commits September 25, 2023 14:31
Co-authored-by: zetashift <rskaraya@gmail.com>
@armanbilge armanbilge merged commit 048d183 into scala-js:main Sep 27, 2023
5 checks passed
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.

4 participants