Get rid of the promise returned by observe

[kenchris]

It seems that the returned promise is a left over from when we depending on the permissions integration, which is now gone.

It makes sense to remove it as it would complicate adding support for observe({ sources: ["gpu"] }) given we would probably have to return an array of promises and it opens up for a lot of confusion.

@Elchi3

[kenchris]

• The promise existed due to now removed permission integration
• If we remove promise now, we can always add later without breaking existing code as no developer is depending on it
• If we, in the future, support multiple sources in one observe call we have multiple options, none good
  • Resolve promise when all succeeds, but what to do it one or more fail? I assume devs will would want it to work for working sources
  • Return array of promises - very ugly and unexpected for developers
• UI wise, devs can use a timer and change UI if no events are delivered after a certain threshold
• We could also add an error event like Generic Sensors, but should look at what other similar APIs are doing today

Generally, I think we should remove the promise before shipping - it feels like the safest bet

[kenchris]

So the spec patch is quite simple (I have one local).

Problem is our implementation and testing. We only active the backend when needed and that could in theory fail but is not expected to, anymore than the browser compositor thread could fail :-)

Also users just observe and get events as soon as the system can deliver them. If the pressure source is supported and requirements (like focused window) are met, then events are expected to be delivered.

I thought about wrapping things in a promise and adding a little timeout in the case things break, like

function asyncObserve(callback, ...args) {
  return new Promise((resolve, reject) => {
    const id = setTimeout(reject, 4_000);
    const observer = new PressureObserver(cargs => {
      clearTimeout(id);
      resolve();
      callback(...cargs);
    })
    observer.observe(...args)
  })
}

const promise = asyncObserve(console.log, "cpu")

But @arskama says we cannot use setTimeout in tests. Not sure what the other best way would be for generic upstreamable tests.

@rakuco

Could we send a failure event just for tests? Potentially we could send the "failure" state or similar

I guess we could also give the PressureObserver a second failureCallback

function asyncObserve(callback, ...args) {
  return new Promise((resolve, reject) => {
    const observer = new PressureObserver(cargs => {
      resolve();
      callback(...cargs);
    }, reject)
    observer.observe(...args)
  })
}

[kenchris]

I looked at https://w3c.github.io/webdriver-bidi/ and it seems that we can probably use that for testing this. @rakuco

I am not sure that I really understand how it works, but something like

pressureObserver.ObserveStarted= (
 method: "pressureObserver.observeStarted",
)

pressureObserver.ObserveError= (
 method: "pressureObserver.observeError",
 params: pressureObserver.ObserveErrorParameters,
)

pressureObserver.ObserveErrorParameters = {
  errorText: text,
}

[rakuco]

Some quick answers first:

• It's not that setTimeout() is not allowed in tests; I think @arskama was referring to using t.step_timeout() in web-platform-tests instead of calls to setTimeout(). But even then, the bigger problem is that using timeouts to detect that something did not happen is very flaky: timeout values often do not work well in CI, and it makes testing very unpredictable.
• WebDriver BiDi is the bidirectional version of the WebDriver protocol. It's under development, and the tooling in web-platform-tests is not there yet (i.e. one cannot use testdriver.js with BiDi). In any case...
• ... Having "events just for tests" is generally not a good idea. There shouldn't be special behavior that only happens in tests, otherwise you're not really testing the actual API. Furthermore, even if we did use WebDriver BiDi and defined a way to send a notification if something failed, users would probably want access to the same information anyway (it's the whole point about timeouts being arbitrary above).

Now looking at other Observer-style APIs, I think this one is inevitably more asynchronous than the others, and observe() needs to check if a given source type is supported by the platform or not in an asynchronous manner -- my understanding is that the supportedSources attribute lists the types that can be supported by an implementation, but whether the hardware or OS-level support (together with any permission mechanisms that might exist outside the scope of this API) is only checked when observe() is called.

So if we take into account that there's one asynchronous step that we can't get rid of, and there might be others in the future, maybe getting rid of the promise is not a good idea. The API could take a failure callback like you described, or it could fire an "error" event if something in observe() goes wrong, but it feels like this is going against the design asynchronous APIs using Promises TAG advice.

The problem of the ergonomics of supporting multiple sources in one call is real though, but I wonder if any of the other solutions above would solve this problem better.

Would it be that important to support multiple sources in one call instead of having developers do something like Promise.all([observer.observe('gpu'), observer.observe('thermal', options), observer.observe('gpu', options)])?

[kenchris]

Would be great with @Elchi3 input here.

[Elchi3]

Not sure I can contribute much other than making comparisons to the Performance API :)

Also users just observe and get events as soon as the system can deliver them.

So, I guess to me this is a limitation in PressureObserver. In PerformanceObserver you have the option to retrieve data from a buffer using the buffered option in observe(). This allows you to look at data occurred before observation basically and not only just-in-time. So, if the buffer is empty, nothing happened before... Is that what you want to check for in tests?

or it could fire an "error" event if something in observe() goes wrong

Again, drawing the comparison to the PerformanceObserver, it seems that nothing happens. "Unrecognized types are ignored, though the browser may output a warning message to the console to help developers debug their code. If no valid types are found, observe() has no effect."

Would it be that important to support multiple sources in one call instead of having developers do something like Promise.all([observer.observe('gpu'), observer.observe('thermal', options), observer.observe('gpu', options)])?

I think that might work? I guess the multiple sources use case is basically: "I want to check all available sources and if I see pressure anywhere, I want to do something about it or log it for analytics". And because PressureRecord.source is available I can filter the data as needed later on, so I might as well ask the system for everything it can possibly have for me.

[kenchris]

We don't need to check for data before observing.

I guess people could just do

  const options = { sampleInterval: 2_000 };
  await Promise.all(PressureObserver.supportedSources.map(source => observer.observe(source, options)))

[rakuco]

Promise.all() might not be the best solution in this specific case because it wouldn't be easy to know which observe() call failed if we have e.g. 3 entries in supportedSources, but the idea of using supportedSources to know which sources to pass to observe() looks reasonable to me.

[kenchris]

We could, in the future, send metadata with the resolved promise, but users can also do that easily themselves

  const options = { sampleInterval: 2_000 };
  await Promise.all(PressureObserver.supportedSources.map(source => {
    return observer.observe(source, options).then(_ => source)
  })

[kenchris]

Maybe the above (or something similar) could serve as an example on MDN @Elchi3

I think we can close this.