whatwg / webidl

Web IDL Standard
https://webidl.spec.whatwg.org/
Other
403 stars 162 forks source link

Need pattern for feature detecting dictionary members #107

Open RByers opened 8 years ago

RByers commented 8 years ago

Many new APIs (and some new arguments to existing APIs) are relying on dictionaries. But doing feature detection of such members requires ugly and complex code like:

var supportsCaptureOption = false;
try {
  addEventListener("test", null, Object.defineProperty({}, 'capture', {get: function () {
    supportsCaptureOption = true;
  }}));
} catch(e) {}

This increases the concern that new APIs will lead to sites being broken on older browsers because developers didn't understand or couldn't be bothered with the difficult feature detection.

In WICG/EventListenerOptions#31 @tabatkins proposed a mechanism whereby all dictionary types would automatically get a JS-exposed object with a property-per-member to enable consistent and easy feature detection.

Thoughts?

domenic commented 8 years ago

I'm a little scared of this idea since dictionaries right now are not "reified" in any way. Their names are for spec purposes only, and can be changed at will. They just represent normal JavaScript objects that authors pass in. Having there be a property on the global for each dictionary, which is going to be some type of... object? array? of supported property keys (if object, what is their value?) is pretty weird.

I don't really have any other great solution though. Something like window.dictionarySupports("EventListenerOptions", "passive") isn't great either.

tabatkins commented 8 years ago

(None of this would be necessary if JS hadn't punted on the named-arguments thing. If we had named arguments like Python, this would all be way easier - methods would just throw if you passed a new argument in an old browser, like they do for new positional arguments today. Ugh.)

To be specific, if you define a dictionary like:

dictionary InterfaceMethodOptions {
  DOMString foo = "";
  long long bar = 0;
}

This would define an InterfaceMethodOptions value on the global shaped like:

window.InterfaceMethodOptions = {foo: true, bar: true};

By making this happen automatically in IDL, we get reasonably dependable support, without needing to rely on impls (a) remembering to update their "is this supported?" code, and (b) not lying. This is similar to how @supports works "automatically" by just basing itself on whether the value parses or not.

domenic commented 8 years ago

Can you say why that's better than one of

window.EventListenerOptions = new Set("foo", "bar");
window.EventListenerOptions = ["foo", "bar"];

?

bzbarsky commented 8 years ago

We would definitely need to audit dictionary names in the platform to make sure that none of them have names that are likely to collide with real-world stuff.... Past that this is probably OK, but I agree it should not be raw object. And the reason it shouldn't be is that we don't want to things to go sideways if a dictionary has a "toString" member or whatnot. So a Set is probably a better idea.

tabatkins commented 8 years ago

Sure, I don't have strong opinions on the exact shape. A Set works for me.

jan-ivar commented 8 years ago

FWIW we had this exact problem with getUserMedia(constraints). We ended up defining navigator.mediaDevices.getSupportedConstraints(). The implementation was quite trivial. :)

Still, that seems like a lot of bloat on the window.

jan-ivar commented 8 years ago

I suppose we would only need this for dictionaries that are taken as inputs? Some specs have lots of dictionaries that are only ever returned to content.

annevk commented 8 years ago

I kind of prefer the dictionarySupports() version over defining lots of additional properties on the global. With a method we could also potentially extend it in the future so you can check whether your particular value is supported for a member or not.

tabatkins commented 8 years ago

Just so long as it's something that can reasonably be done automagically by our IDL handlers.

annevk commented 8 years ago

Yeah, it would totally be IDL-driven. We will probably need to start annotating dictionaries with something akin to Exposed. Otherwise IDL code will need to find where the dictionary is used in order to know what global it's supported on (maybe that's okay?).

Another thing is that we should indeed probably not do this for dictionaries that are solely returned. It only makes sense for those accepted as an argument somewhere. That probably requires an annotation or again some finding "magic".

RByers commented 8 years ago

Why don't we start by adding an extended-attribute that opts dictionaries into this behavior? We can try that out in a few cases, then revisit if/how to change the default.

bzbarsky commented 8 years ago

That probably requires an annotation or again some finding "magic".

You can't do this without annotations in general: there are some APIs that take object and examine something on it to decide which dictionary type to convert it to...

jan-ivar commented 8 years ago

At the risk of sounding spoiled, I think I would expect the webidl compiler to do this automatically whenever it can (which should be most of the time?), and instead require annotation when using dictionaries in unobvious ways. I worry most spec writers would forget otherwise.

tabatkins commented 8 years ago

Yeah, I don't see why return-only dictionaries are a problem here. It's not useful to feature-detect them (probably, tho I could imagine some cases), but if leaving them out means we have to affirmatively annotate dictionaries we want in, it's not worth the trouble - we should just put them all in.

I'm fine with the dictionaries using the same [Global] defaulting that interfaces use.

sicking commented 8 years ago

FWIW, this problem applies to enum values in addition to dictionary members.

tabatkins commented 8 years ago

Good point. Do enums live in the same or different namespace from dictionaries?

bzbarsky commented 8 years ago

Same namespace, because when you use it as an arg type, you just use the name.

tabatkins commented 8 years ago

Ah, right. I... should probably address that in Bikeshed. (It treats all the name-definers as separate namespaces right now and won't warn you if names collide.)

jan-ivar commented 8 years ago

Isn't enum arg detection straightforward? obj.foo = 'bar'; obj.foo == 'bar'

RByers commented 8 years ago

Not if the only use of the enum is in a method argument (same as the dictionary issues we're discussing).

RByers commented 8 years ago

Do we want to allow enumeration of supported dictionary/enumeration members? I can see arguments either way, without a compelling use case I'd probably prefer we keep this scoped to feature detection.

annevk commented 8 years ago

The other thing I was wondering about is if we are going to expose dictionaries, should we attempt at normalizing their names somehow?

I think we should do enums separately by the way. They are either ignored (setters) or the method throws, which makes them reasonably detectable. Apart from that, they would require a different API.

annevk commented 8 years ago

And yes, agreed that we should keep it simple initially.

jan-ivar commented 8 years ago

@RByers unknown enum args in methods throw.

tabatkins commented 8 years ago

Do we want to allow enumeration of supported dictionary/enumeration members? I can see arguments either way, without a compelling use case I'd probably prefer we keep this scoped to feature detection.

I'm confused - enumerating the supported dictionary members is literally the request here. (Or at least, being able to ask if a given name is a supported dictionary member.) I'm 100% against anything attempting to be smarter such that it can no longer be trivially automated with no human intervention required.

I think we should do enums separately by the way. They are either ignored (setters) or the method throws, which makes them reasonably detectable. Apart from that, they would require a different API.

Why would they require a different API? Afaict they'd have the identical "set of supported names for this type" API.

annevk commented 8 years ago

@tabatkins well, e.g., do enums and dictionary share a namespace? It's also not clear to me why they would be the same API, since you can do much more with dictionaries than simple member checking going forward as I hinted earlier (e.g., seeing whether a member accepts a particular value).

RByers commented 8 years ago

I'm confused - enumerating the supported dictionary members is literally the request here. (Or at least, being able to ask if a given name is a supported dictionary member.)

Right. There have been two main classes of APIs discussed:

  1. dictionarySupports("EventListenerOptions", "passive") - provides feature detection without enumeration
  2. "passive" in EventListenerOptions - provides both feature detection and enumeration

My question was just whether we considered supporting enumeration in addition to feature detection a good or bad thing. I can certainly imagine cases where allowing enumeration causes more problems than benefits. If we don't have any good reason to want to support it, then we should probably prefer the 1) style over the 2) style as a result.

tabatkins commented 8 years ago

well, e.g., do enums and dictionary share a namespace?

Yes, this was asked by me and answered by bz immediately prior to your comment: https://github.com/heycam/webidl/issues/107#issuecomment-211592249 Everything that can be used as an argument type shares a namespace: interfaces, dictionaries, enums, and typedefs. I opened a Bikeshed bug to enforce that more thoroughly.

ddorwin commented 8 years ago

Does this issue address or cover the same use cases as https://www.w3.org/Bugs/Public/show_bug.cgi?id=19936? If so, should we close it with a reference to this issue?

annevk commented 8 years ago

@ddorwin I think the motivation for that bug is different. That is about treating invalid values as another value. This is about testing whether a value is supported.

RByers commented 7 years ago

Note that if we don't agree on something here, we're likely to add but-yet-another one-off API in another special case.

mgiuca commented 7 years ago

We are also thinking about this for Web Share to add a way to feature-detect whether the API supports particular input fields. Right now we are thinking we'll have to convert the dictionary to an interface in order to allow the JavaScript to detect fields.

domenic commented 7 years ago

I had an idea today that I think might actually work. The main sticking point for me is how to avoid exposing the dictionary names to the web; they are currently unobservable, and IMO that is a nice property.

What if we made it easy for dictionary-accepting APIs to get a feature-detection method added to them? I am thinking the following JS code:

createImageBitmap.supports("imageOrientation", "none")
navigator.share.supports("image");

These could be generated by Web IDL such as

interface WindowOrWorkerGlobalScope {
  [WithDictionarySupports] Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, optional ImageBitmapOptions options);
}

partial interface Navigator {
    [SecureContext, WithDictionarySupports] Promise<void> share(ShareData data);
};

The limitation of this is that it assumes a single dictionary argument. Hopefully that is all we'll ever need... You could imagine generalizations that allow more, but they get uglier to use.

RByers commented 7 years ago

I like it!

In your "imageOrientation","none" example that's explicitly testing both that the dictionary member exists, and that the value is a valid member of the enum? Does supports also handle just checking enum members without any dictionary at all?

Would passing a dictionary possibly work better than strings? Eg.

  createImageBitmap.supports({imageOrientation: "none"})

Then in some cases you may just use the same dictionary (or a subset of it) that you'd end up passing to the actual call.

domenic commented 7 years ago

In your "imageOrientation","none" example that's explicitly testing both that the dictionary member exists, and that the value is a valid member of the enum?

Yeah. Off the cuff, the one-argument version is useful for scenarios where the value space is either very specific (e.g. always a Blob) or very general (any, DOMString, etc.). While the two-argument form is useful for a limited value space. The clearest example of a limited value space is enum; in the enum case a supports() method could be code-generated with no additional information (either in spec prose or implementations).

Does supports also handle just checking enum members without any dictionary at all?

I suppose it could, although it makes the "only operates on one argument" issue worse. (I.e. now it doesn't work for methods which take one enumeration argument and one dictionary argument.) I think I'd stick with dictionaries for now? Unless we have a list of platform APIs that need this, in which case we could make a more informed decision.

Would passing a dictionary possibly work better than strings?

Hmm, yeah, this might be better. It is more complicated; e.g. now we have to define what happens with partial support (presumably returns false), or the empty dictionary (presumably returns true). And I'm not sure about how you'd use it for web share; would it be navigator.share.supports({ image: new Blob() })? We should discuss further...

mgiuca commented 7 years ago

@domenic: Yeah I find it aesthetically much nicer to ask a feature-detect question of the API not the dictionary object. I would rather avoid polluting the global namespace with dictionary names just for this purpose.

On navigator.share I was leaning more towards adding a separate canShare method rather than expose the dictionary. Having a standard supports "sub-method" would help avoid polluting the namespace with canX methods.

The limitation of this is that it assumes a single dictionary argument.

If we're going with this approach, I don't see why we would bother tying it to the dictionary at all, or restricting it to dictionary-taking methods. Why not just say certain methods have a "supports" sub-method that takes a single string and returns a Boolean indicating whether that feature is supported. I'm not sure why this "imageOrientation" example has a second argument (what "none" means).

This could be formalized in IDL like this (strawman syntax):

partial interface Navigator {
    [SecureContext, SupportsFeatures<"image", "video">] Promise<void> share(ShareData data);
};

The presence of SupportsFeatures indicates that share.supports exists, and the feature list inside shows what strings are valid arguments to the supports method.

@RByers suggestion works for me also: this generalizes to "the supports method takes exactly the same arguments as the base method, has no side-effect, and returns true if it notionally would accept that argument". For example, you could write:

canvas.toBlob(blob => {
  let sharedata = {title: 'My painting', image: blob};
  if (!navigator.share.supports(sharedata)) {
    // error
  }
  navigator.share(sharedata);
}

So you just call supports with the same argument you would then pass to the API. The downside of this is: how do we communicate "partial support"; for example, a UA that doesn't support sharing images won't fail if it gets a ShareData with a title and image, it'll just share the title. So should navigator.share.supports on a non-image-supporting UA return false if given a title and image, even though it would still partially succeed? Perhaps it's best if I can directly ask the API "do you support image sharing"?

domenic commented 7 years ago

If we're going with this approach, I don't see why we would bother tying it to the dictionary at all, or restricting it to dictionary-taking methods.

Well, part of the idea of this thread is to have it be an easy drop-in for the spec and for implementations so that you can code-generate the method. Indeed the original idea was to auto-generate this for all dictionaries (and enums?) on the platform. My version requires some opt-in per dictionary/dictionary-accepting method, but still code-generates the supports() logic at least.

You seem to be suggesting something that requires more work for spec authors, which is OK. However it also has the downside of possibly being more inconsistent. E.g. it seems like your version of web share doesn't say yes to supports("url"), which seems bad. In other words it sounds like supports("x") will only return true for "x"s added to the spec after some point in time at which the spec authors started caring about feature detection. I think that is not great, compared to automatically synchronizing it with all the keys of a dictionary.

@RByers suggestion works for me also: this generalizes to "the supports method takes exactly the same arguments as the base method, has no side-effect, and returns true if it notionally would accept that argument".

Ah, that helps me understand it better. In particular you could just pass the blob you're planning to pass to share() anyway.

The biggest issue I see is the one you point out about partial support. In particular I can imagine unfortunate coding patterns which end up attempting all combinations of arguments to find the one that has the maximal amount of features the developer wants to use, but still returns true.

The other issue is "no side effects". Web IDL type coercions will cause side effects; indeed you'll perform any type coercions twice. But in most cases, e.g. those not involving getters or one-shot iterators, the type conversion is idempotent...

mgiuca commented 7 years ago

it seems like your version of web share doesn't say yes to supports("url"), which seems bad. In other words it sounds like supports("x") will only return true for "x"s added to the spec after some point in time at which the spec authors started caring about feature detection.

That's valid. I guess you could either add all the dictionary members (but then your IDL is going to have a redundancy where every dictionary key is also listed in the SupportsFeatures bit), or you could say there are some basic things that any implementation really needs to have to be compliant (for navigator.share, that's title, text and url) and then there are some "add-on" features that you can omit.

Ah, that helps me understand it better.

Note I am not trying to put words in @RByers mouth; he may have not been thinking of literally passing the full dictionary.

Yeah the partial support issue feels like a deal-breaker, at least for Web Share.

Web IDL type coercions will cause side effects

Ouch, if that can be arbitrary side effects then I'd rather avoid it.

domenic commented 7 years ago

Ouch, if that can be arbitrary side effects then I'd rather avoid it.

Yeah, consider

const shareOptions = {
  get title() {
    console.log("foo");
    return "title";
  }
};

if (navigator.share.supports(shareOptions)) { // will log
  navigator.share(shareOptions); // will log again
}
junov commented 7 years ago

I like the supports() proposal. Here is another idea I would just like to throw into the ring...

The fact that unsupported dictionary members are silently ignored is generally nice for compatibility... Until we encounter an option that the web app can't live without. Basically, there are options that fall into the 'like to have' category and others that are required by the web app. Right now, everything is treated as like-to-have. What if we added some form of notation to indicate options that are strictly required. I'm not sure how exactly we'd do this. Here's a straw man proposal: what if a '+' prefix in front of a dictionary key indicated a required option. Then we could do something like this:

createImageBimap(myBlob, { +imageOrientation: "flipY", colorSpaceConversion: "none" }).then(onLoad, callFlipYPolyfill);

Basically, the call would be forced to fail (in this case reject the promise) when a required option is not supported. Obviously, there would have to be a way to feature detect the special notation.

domenic commented 7 years ago

@junov interesting, although perhaps a separate issue?

In the interest of scoping this proposal, maybe the first version should be a single-arg supports (no enum detection), applicable to methods with one (possibly optional) dictionary argument.

I think the biggest help we could get for moving this forward is a list of APIs that would use it. If we have a reasonable list, we can validate my proposal against it, and compare with @RByers's dictionary-accepting version, and then proceed to spec. Could others help assemble that?

junov commented 7 years ago

For createImageBitmap() and HTMLCanvasElement.getContext(), enum detection is rather important. See my comment here: https://github.com/whatwg/html/issues/2610#issuecomment-298654841

fserb commented 7 years ago

Could you clarify a bit what would be the semantics of <class>.supports()? Does it just mean "it's on my IDL", i.e. no actual support is necessary/guaranteed? Do you think it would be confusing to do:

const myOptions = {...};
if (whatever.support(myOptions)) {
  let a = whatever(myOptions); 
}

and then a fails to be created? Or worse, if myOptions are actually ignored when creating it?

And if the meaning of it is "actually supported", it means it should never be ignored? (If we are looking at this as a feature detection). Otherwise, we wouldn't be really addressing it and supports would be more of a necessary but sufficient condition? And if so, then I'm not sure I get the utility of this outside of another unreliable feature detection scheme.

My feeling is that we seem to be trying to solve an issue of required/optional parameters, which the "have an options dictionary argument" created.

junov commented 7 years ago

@fserb: I think that in the cases where there are discrepancies between the implementation's IDL and what is actually supported, this can be simply handled in the implementation by overriding auto-generated supports() with a custom implementation of supports(). I think these cases would be quite rare (options only supported under specific conditions?).

fserb commented 7 years ago

@junov: So you are saying that a feature that is supports()ed, shouldn't be ignored, right? But then, what's the advantage of this over if (!createImageBitmap(..., {myCrazyOption:}))?

RByers commented 7 years ago

I didn't suggest the full "pass the exact same argument list" design because I do think the semantics are different. We don't want to imply that the method is doing some arbitrary argument validation. It's simply testing whether the dictionary (and enum) members are recognized or not. I'm also fine with @domenic's simple 2-string-arg version if that's easier to get consensus on.

In terms of where this would be used, here's a specific list that I can keep updated of the specific APIs we know would benefit (and I'll update this comment if more are mentioned).

Interface method dictionary notes
EventTarget addEventListener AddEventListenerOptions started this debate
Navigator share ShareData
ImageBitmapFactories createImageBitmap ImageBitmapOptions needs enum values too
HTMLCanvasElement getContext CanvasContextCreationAttributes
MediaStreamTrack applyConstraints MediaTrackConstraints supercedes getSupportedConstraints

But I don't know how to find the exhaustive set of cases. I guess it ultimately comes down to how often we extend a dictionary or add new dictionary-taking overloads in non-optional ways. I can't think of any automated way to find those cases.

But in case it helps, here's a list of other `dictionary` types currently in blink (not necessarily all used by shipping APIs), excluding ones that I think probably wouldn't benefit (eg. initializer dictionaries - where you can just feature detect the type of object being initialized). For many of these, if we ever have or want to in the future extend the dictionary, `supports` could be valuable. ``` AnalyserOptions AndroidPayMethodData AndroidPayTokenization AnimationEffectTimingProperties AssignedNodesOptions AudioBufferOptions AudioBufferSourceOptions AudioConfiguration AudioContextOptions AudioNodeOptions AudioTimestamp AuthenticationAssertionOptions AuthenticationClientData AuthenticationExtensions BackgroundFetchOptions BasicCardRequest BlobPropertyBag CacheQueryOptions CanvasContextCreationAttributes CanvasRenderingContext2DSettings ClientQueryOptions ComputedTimingProperties ConstantSourceOptions ConstrainBooleanParameters ConstrainDOMStringParameters ConstrainDoubleRange ConstrainLongRange ConstrainPoint2DParameters CredentialCreationOptions CredentialData CredentialRequestOptions CSSCalcDictionary ElementCreationOptions ElementDefinitionOptions ElementRegistrationOptions FaceDetectorOptions FederatedCredentialRequestOptions FilePropertyBag FileSystemFlags FontFaceDescriptors ForeignFetchOptions ForeignFetchResponse FormDataOptions GetNotificationOptions GetRootNodeOptions HitRegionOptions IconDefinition IDBIndexParameters IDBObjectStoreParameters IdleRequestOptions ImageDataColorSettings ImageEncodeOptions InternalDictionaryDerived InternalDictionaryDerivedDerived KeyframeAnimationOptions KeyframeEffectOptions Landmark LongRange MediaConfiguration MediaDecodingConfiguration MediaElementAudioSourceOptions MediaEncodingConfiguration MediaImage MediaKeySystemConfiguration MediaKeySystemMediaCapability MediaRecorderOptions MediaStreamAudioSourceOptions MediaStreamConstraints MediaTrackCapabilities MediaTrackConstraints MediaTrackConstraintSet MediaTrackSettings MediaTrackSupportedConstraints MIDIOptions MidiPermissionDescriptor NavigationPreloadState NFCMessage NFCPushOptions NFCRecord NFCWatchOptions NotificationAction NotificationOptions PannerOptions PasswordCredentialData PaymentAppRequest PaymentAppResponse PaymentCurrencyAmount PaymentDetailsBase PaymentDetailsModifier PaymentDetailsUpdate PaymentInstrument PaymentItem PaymentMethodData PaymentOptions PaymentShippingOption PerformanceObserverInit PeriodicWaveConstraints PeriodicWaveOptions PermissionDescriptor PhotoSettings Point2D PositionOptions PropertyDescriptor PushPermissionDescriptor RegistrationOptions RelyingPartyAccount RequestDeviceOptions RTCAnswerOptions RTCConfiguration RTCIceServer RTCOfferAnswerOptions RTCOfferOptions ScopedCredentialDescriptor ScopedCredentialOptions ScopedCredentialParameters ScrollOptions ScrollToOptions SensorOptions StorageEstimate TextDecodeOptions TextDecoderOptions USBControlTransferParameters USBDeviceFilter USBDeviceRequestOptions VideoConfiguration VRLayer WebGLContextAttributes WorkletOptions ```
RByers commented 7 years ago

The biggest issue I see is the one you point out about partial support. In particular I can imagine unfortunate coding patterns which end up attempting all combinations of arguments to find the one that has the maximal amount of features the developer wants to use, but still returns true.

I was expecting supports would return true when all of the supplied dictionary members or enum values were known (but wouldn't, for example, return false just because a required member wasn't supplied - supports doesn't actually take the dictionary type itself). Then if a developer wants to determine the full set of supported options, she needs to call supports once for each option she's interested in. I don't think there'd be any combinatorial scenario, i.e.:

  (foo.supports({a:true}) && foo.supports({b:true}) ===
    foo.supports({a:true, b:true})
tabatkins commented 7 years ago

I don't understand why we're squeamish about exposing dictionary names to the web. The names are already required to be unique in the WebIDL ecosystem, as they live in the same namespace as interfaces. The names are also usually quite long and weird, so collision chance is low.

I'm strongly against something that requires spec authors to affirmatively annotate their dictionary-taking APIs to make them dict-member-detectable, because that just means that most dict-taking APIs won't have it, and authors will have to remember yet another detail of the API (whether or not it exposes that feature-detection) before they try to use it, and there'll be browser differences in whether it's supported or not in a particular browser/version... All of this is bad and unnecessary when we can just somehow globally expose all dictionaries.

I don't care if we do it with a single global function like IDLDictSupports("dictName", "memberName") instead of adding a bunch of dictionary names to the global, I just care that it works for everything automatically.

tabatkins commented 7 years ago

And anything that attaches the "is supported?" functionality to a given method assumes that methods will never take more than one dictionary argument, which is a broken assumption.

mgiuca commented 7 years ago

Yeah, if this is going to be specifically about feature-detecting whether an impl. supports a dictionary key, then I don't see why we would want to tie it to a particular method.

In addition to the multiple-dicts-per-method problem, you could also imagine two methods taking the same dictionary:

dictionary MyDict {
  int mykey;
}

partial interface Navigator {
  void method1(MyDict options);
  void method2(MyDict options);
}

Then the developer has to decide which method to enquire about (even though they both return the same answer), and possibly get confused about whether they need to perform the enquiry twice:

if (method1.supports('mykey')) {
  method1({mykey: ...});
}
if (method2.supports('mykey')) {
  // Is the above check redundant? Can I just combine this into a single if statement?
  method2({mykey: ...});
}

Probably best to just allow feature detection on the dict itself, not the method.