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

Consider changing the spec text to allow buffered with entryTypes #215

Open
hiikezoe opened this issue Jan 8, 2025 · 5 comments
Open

Consider changing the spec text to allow buffered with entryTypes #215

hiikezoe opened this issue Jan 8, 2025 · 5 comments

Comments

@hiikezoe
Copy link

hiikezoe commented Jan 8, 2025

The spec text in question is here in observe() method section at the 3

  1. If options's entryTypes is present and any other member is also present, then throw a "TypeError".

The text means the correct behavior here is to throw a TypeError if there are both of buffered and entryTypes, but either Blink or WebKit doesn't throw the exception. It behaves as if there's no buffered option specified.

On the other hand, Gecko does throw the exception. But it is going to be changed to align with the other browser engine since there's a severe bug report (bug 1915589) that a shopping site isn't usable at all because of the exception.

That's being said, I personally prefer the throwing the exception behavior because from web developers perspective buffered can be specified with entryTypes but buffered option is ignored, it would be confusing. So I am okay to keep this issue open until Chrome's usage counter of the case reaches to a point that we can throw the exception in the case.

CCing @noamr

There's an open interop 2025 for this behavior difference. web-platform-tests/interop#856

@mmocny
Copy link

mmocny commented Feb 25, 2025

Thanks for filing this and for linking to the interop issues.

It's not clear to me which of the following options are most desired for everyone to interop around:

  • Should all implementations just support using entryTypes + buffered/durationThreshold, apply the values only for those entry types which support it, and ignore any entry types that don't support it (i.e. silent fallback), or
  • Should all implementations just support using entryTypes + buffered/durationThreshold, but throw if ANY of the entry types listed do not support any of the options provided
  • Should we do what is specced right now and not support this combo at all, even if the listed entryTypes could all support the options

I understand the request to interop around some consistent behaviour, and I think any of these options are viable -- the first seems more flexible and "fine", to me -- but the last is consistent with existing spec and most conservative. It's not a big deal to ask authors to make separate .observe() calls, and I also think its not that common to share PerformanceObservers with a list of entry types (citation needed...).

@noamr
Copy link
Contributor

noamr commented Feb 25, 2025

Thanks for filing this and for linking to the interop issues.

It's not clear to me which of the following options are most desired for everyone to interop around:

  • Should all implementations just support using entryTypes + buffered/durationThreshold, apply the values only for those entry types which support it, and ignore any entry types that don't support it (i.e. silent fallback), or
  • Should all implementations just support using entryTypes + buffered/durationThreshold, but throw if ANY of the entry types listed do not support any of the options provided
  • Should we do what is specced right now and not support this combo at all, even if the listed entryTypes could all support the options

I understand the request to interop around some consistent behaviour, and I think any of these options are viable -- the first seems more flexible and "fine", to me -- but the last is consistent with existing spec and most conservative. It's not a big deal to ask authors to make separate .observe() calls, and I also think its not that common to share PerformanceObservers with a list of entry types (citation needed...).

There's an option 4, which is to treat {entryTypes: [onlyOneType]} the same as {type: onlyOneType}. I think it would fix the current interop issue without creating any regressions.

@mmocny
Copy link

mmocny commented Feb 26, 2025

A few more notes:

  • While the current spec text says that we should throw an error, the existing WPT tests already expect that we don't throw.
  • They also test that we ignore the buffered option, and all implementations pass.
  • Changing to the specced behaviour would break existing sites, (as it seems Mozilla folks learned, and fixed already). Thus the specced behaviour seems no longer viable.

Given that, I suspect we should at least:

  • Update spec to remove the "throw" requirement

Beyond the basics:

  • entryTypes only checks for the buffered option, not other options like durationThreshold. It's fine to provide a durationThreshold on all entry types. When provided with entryTypes: ['event'], it seems to also get ignored (like buffered).
  • Looking at the registry, and comparing to the list of PerformanceObserver.supportedEntryTypes, are there even any entries that don't actually support the buffered option?

Given all that, I suspect we can additionally just support entryTypes as equivalent to a loop registering each type with all the options. It would already be supported without warnings or errors if you did that.

@mmocny
Copy link

mmocny commented Feb 26, 2025

More historical context: #103

@mmocny
Copy link

mmocny commented Feb 26, 2025

Feedback from WG call:

  • For now, the spec is not interoperable with all implementations, so lets make the spec change to match implementations (and to match WPT and interop 2025)
  • Once fixed, convert this issue (or open a new one) for the feature request of also NOT ignoring the buffered option (or other options) when used with entryTypes, especially since it looks like all entry types do support buffering at least 1 value now (and since the type format will accept it for all entries, silently)

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

No branches or pull requests

3 participants