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

Move source to options #262

Closed
kenchris opened this issue Apr 8, 2024 · 7 comments
Closed

Move source to options #262

kenchris opened this issue Apr 8, 2024 · 7 comments

Comments

@kenchris
Copy link
Contributor

kenchris commented Apr 8, 2024

Following up on @Elchi3 's #256 where the following was suggested

await observer.observe({ sources: ["cpu"], sampleInterval: 2500 });

This is pretty consistent with PerformanceObserver, the "type" (and "entryTypes") is also part of the options.

We could do

await observer.observe({ source: "cpu", sampleInterval: 2500 });

now (pre shipping), and add sources in the future.

Or does @Elchi3 believe we should just support sources and not source?

@Elchi3
Copy link

Elchi3 commented Apr 8, 2024

I think the reason for PerformanceObserve to have both type and entryTypes is that there are rules about how this plays together with other properties of the options object:

// valid
observe({ type: "resource", buffered: true });
observe({ type: "event", durationThreshold: 160 });
observe({ entryTypes: ["mark", "measure"] });

// invalid
observe({ entryTypes: ["resource"], buffered: true });
observe({ entryTypes: ["event", "mark"], durationThreshold: 160 });
observe({ entryTypes: ["resource"], type: "event" });

So in PressureObserver, you need to decide if you want to allow sampleInterval to be set for all sources or not, for example, and what would be the deal with source-specific options:

await observer.observe({ sources: ["cpu", "gpu"], sampleInterval: 2500 }); / valid?
await observer.observe({ source: "cpu", sampleInterval: 2500 }); // valid

await observer.observe({ sources: ["cpu", "gpu"], gpuSpecificOption: "foo" }); // invalid?
await observer.observe({ source: "gpu", gpuSpecificOption: "foo" }); // valid

Or does @Elchi3 believe we should just support sources and not source?

I think I would say having both makes sense. Especially, if you believe there will be source-specific options.

@kenchris
Copy link
Contributor Author

kenchris commented Apr 8, 2024

sampleInterval should be available for all sources and are a request, given the hardware might not be able to fulfill it.

I think it is a bit early to say if we get source specific options, but that could definitely happen.

I generally think that

  observer.observe({ source: "gpu" })

is more clear than

  observer.observe("gpu")

@kenchris
Copy link
Contributor Author

@rakuco do you have any input?

@rakuco
Copy link
Member

rakuco commented Apr 11, 2024

My understanding is that "source" (or even "sources" depending on the outcome of #258 and the above) will always be a required parameter given the semantics of this API. This is similar to element or target in MutationObserver, IntersectionObserver and ResizeObserver, so from a spec or IDL perspective it feels confusing to have it as part of PressureObserverOptions, as options is an optional parameter for observe().

You could also make it a required PressureObserverOptions member and stop making options optional, but it feels contrary to https://w3ctag.github.io/design-principles/#optional-parameters (other parameters are indeed optional) and https://w3ctag.github.io/design-principles/#prefer-dictionaries.

@kenchris
Copy link
Contributor Author

That is exactly what it was modelled after @rakuco I just wanted to make sure that we really thought this through. Maybe PerformanceObserver is a bit of the odd one out here.

I think we are good and should close this.

@tomayac
Copy link
Contributor

tomayac commented Apr 12, 2024

To be on the safe side, it may be worthwhile to loop in the @w3ctag and hear their opinion on the matter.

@kenchris
Copy link
Contributor Author

True, if the @w3ctag disagree with our reasoning, please reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants