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

Simple EventListenerOptions feature detection #491

Closed
RByers opened this issue Aug 12, 2017 · 16 comments
Closed

Simple EventListenerOptions feature detection #491

RByers opened this issue Aug 12, 2017 · 16 comments

Comments

@RByers
Copy link
Contributor

RByers commented Aug 12, 2017

After over a year of debating it we seem no closer to having consensus on a generic mechanism for dictionary feature detection. In the meantime I keep hearing people site the difficulty of feature detection as the biggest problem with passive event listeners. For example @cramforce keeps using this as an example of why performance-sensitive applications often need to do browser-conditional-behavior based on UA-sniffing instead of feature detection.

I'd love to see a generic solution, but I don't have the expertise or time to keep pushing the debate. We've got other APIs already that work around this problem, like mediadevices.getSupportedConstraints. I suggest that for now we just perpetuate that pattern by adding:

  interface EventTarget {
    static AddEventListenerOptions getSupportedOptions();
  }

Which returns a dictionary with each supported option value set to true. Thoughts?
@annevk @dtapuska @tabatkins @domenic @bzbarsky

@RByers
Copy link
Contributor Author

RByers commented Aug 12, 2017

Oh and as I understand it, @cramforce's performance concern is that in large systems you really don't want each independent module creating GC pressure by allocating memory during initialization just for feature detection. Maybe returning a new dictionary is a bad thing by that metric?

I guess I'd rather not introduce another one-off pattern if the rough pattern used by getSupportedConstraints isn't good enough. But maybe as the interim solution it's fine. Presumably some day we'll have a generic solution in WebIDL and use of these ad-hoc feature detection APIs can be discouraged.

@domenic
Copy link
Member

domenic commented Aug 12, 2017

I'd be interested to see some performance data on @cramforce's concern.

@cramforce
Copy link

I don't care so much about performance here (we don't do feature detection that allocates non-trivial memory to avoid death by a thousand cuts, and we never throw exceptions in feature detection, so that page loads with "break on caught exceptions" don't break on non-exceptions). To directly address the request for "data". Of course, the individual detection doesn't take long, but larger apps need to detect many features. Altogether this can easily take dozens of milliseconds, which is completely unacceptable.

It is just that feature detecting this is just plain ridiculous: Imagine you teach a class about events, and the first thing people have to learn is: This is how you observe whether a property has been read in JavaScript. You can literally write GMail without knowing about that JavaScript feature if it wasn't needed for feature detecting whether event listeners support the passive option.

Meanwhile we have have global objects like PerformancePaintTiming that serve zero purpose, but they do allow inferring that PerformanceObserver will emit 'first-contentful-paint' entries. That is great. relList.supports is another good example where developer ergonomics where considered in API design.

No feature on the web platform should be launched without making it detectable whether it is support in some reasonable fashion.

@domenic
Copy link
Member

domenic commented Aug 13, 2017

I think we have different definitions of reasonable. I don't think burdening the web platform with yet another way of doing feature detection is reasonable, at least until we hear about real-world performance problems caused in apps by this.

@cramforce
Copy link

cramforce commented Aug 13, 2017

You really think that this is reasonable?

    let passiveSupported = false;
    const options = Object.defineProperty({}, 'passive', {
      get: function() {
        passiveSupported = true;
      },
    });
    self.addEventListener('test-passive', null, options);

I'm not asking for a new way of doing feature detection. Just making EventTarget.EventListenerOptions available would solve half the problem.

This API is especially bad in its evolution, since one MUST use feature detection because it is using the third arg to addEventListener in a non-backward compatible way.

@annevk
Copy link
Member

annevk commented Aug 13, 2017

Yeah, that's how you can detect dictionary member support for a large number of APIs. Why is it unreasonable? Also, why don't you use an object literal? Seems a little simpler.

@cramforce
Copy link

cramforce commented Aug 13, 2017 via email

@annevk
Copy link
Member

annevk commented Aug 14, 2017

Okay, I guess we disagree on what is unreasonable then. Discovering new features in platform APIs very often requires calling the API and either catching an exception if the feature is not supported or doing something like the above technique for dictionaries. Over time you can remove the discovery phase as those features become ubiquitous.

@tabatkins
Copy link
Contributor

I strongly agree with @cramforce that this nasty trick for detecting whether a key is supported is not reasonable. It's very complex, using a corner of JS that there's virtually no reason to go into for 99% of JS users, and it relies on there being a way to call the API that has no side-effects, which is not something we can depend on all such APIs having in the future. (For example, finding if a new requestFullscreen() option is supported would... request fullscreening. That's not acceptable.)

This is terrible and untenable. It really does need to be fixed.

Again, we get 95% of the way to a solution (which is quite good enough here, people can do more complicated things for more complicated cases) solely by exposing the set of names supported in a particular dictionary. This is how the majority of existing feature detection works already - just checking if a given property exists on an interface - and it's totally fine; false positive rate is fairly low, as browses are incentivized to not expose broken/incomplete versions of APIs, because people immediately and loudly complain about it breaking their feature detection. ^_^

I support ELO doing a one-off solution for now, and us continuing to pursue a generic solution here for exposing the set of names.

@annevk
Copy link
Member

annevk commented Aug 15, 2017

Even requestFullscreen() can be invoked with the full knowledge that it won't succeed (and yet you'd still find out whether your theoretical option would be supported).

Anyway, I don't object to a generic solution per se, but this thread is not about that.

@tabatkins
Copy link
Contributor

Right, it's about whether ELO should have a specialized one-off detection solution, in advance of a generic solution being created. You and Domenic are arguing that no detection solution is required at all, because it's technically possible to figure out whether an option is supported. I'm supporting Malte in disagreeing strongly with that.

@domenic
Copy link
Member

domenic commented Aug 15, 2017

No, I at least am arguing that there's no need to create a one-off solution, because we already have a one-off solution for ELO. It's not aesthetically pleasing, but neither is creating one-off solutions in general.

@cramforce
Copy link

Can anyone explain to me why we have 12 unneeded globals named Performance* but cannot make EventTarget.EventListenerOptions if only to determine that the backwards incompatible API is supported? I've never really seen a need to feature detect support for passive. As long as you know that the options argument is supported you can set it to true. Browsers that don't support passive will gracefully fallback.

@domenic
Copy link
Member

domenic commented Aug 15, 2017

Because nobody was paying attention when those unneeded performance globals were added, possibly.

@bzbarsky
Copy link

More like because we assume that all interfaces should be exposed on the global, e.g. so you can reach their prototypes and do instanceof checks and the like. So it's not very fair to say they're "unneeded".

In practical terms, the infrastructure for adding new interfaces has been in place for close to two decades in browsers and specs, so it's easy. Adding something like EventTarget.EventListenerOptions requires new machinery both in the spec and in all browsers, so it's a little more complicated, even ignoring any disagreement about whether it's the right approach.

@annevk
Copy link
Member

annevk commented Mar 13, 2018

I'm going to close this per the arguments given earlier. It seems unlikely we'll come to an agreement on this.

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

No branches or pull requests

6 participants