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

Need pattern for feature detecting dictionary members #107

Open
RByers opened this issue Apr 8, 2016 · 79 comments
Open

Need pattern for feature detecting dictionary members #107

RByers opened this issue Apr 8, 2016 · 79 comments

Comments

@RByers
Copy link

@RByers RByers commented Apr 8, 2016

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
Copy link
Collaborator

@domenic domenic commented Apr 8, 2016

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
Copy link
Contributor

@tabatkins tabatkins commented Apr 8, 2016

(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
Copy link
Collaborator

@domenic domenic commented Apr 8, 2016

Can you say why that's better than one of

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

?

@bzbarsky
Copy link
Collaborator

@bzbarsky bzbarsky commented Apr 8, 2016

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
Copy link
Contributor

@tabatkins tabatkins commented Apr 8, 2016

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

@jan-ivar
Copy link
Contributor

@jan-ivar jan-ivar commented Apr 8, 2016

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
Copy link
Contributor

@jan-ivar jan-ivar commented Apr 8, 2016

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
Copy link
Collaborator

@annevk annevk commented Apr 9, 2016

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
Copy link
Contributor

@tabatkins tabatkins commented Apr 9, 2016

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

@annevk
Copy link
Collaborator

@annevk annevk commented Apr 9, 2016

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
Copy link
Author

@RByers RByers commented Apr 9, 2016

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
Copy link
Collaborator

@bzbarsky bzbarsky commented Apr 9, 2016

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
Copy link
Contributor

@jan-ivar jan-ivar commented Apr 9, 2016

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
Copy link
Contributor

@tabatkins tabatkins commented Apr 9, 2016

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
Copy link

@sicking sicking commented Apr 18, 2016

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

@tabatkins
Copy link
Contributor

@tabatkins tabatkins commented Apr 18, 2016

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

@bzbarsky
Copy link
Collaborator

@bzbarsky bzbarsky commented Apr 18, 2016

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

@tabatkins
Copy link
Contributor

@tabatkins tabatkins commented Apr 18, 2016

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
Copy link
Contributor

@jan-ivar jan-ivar commented Apr 19, 2016

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

@RByers
Copy link
Author

@RByers RByers commented Apr 19, 2016

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

@RByers
Copy link
Author

@RByers RByers commented Apr 19, 2016

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
Copy link
Collaborator

@annevk annevk commented Apr 19, 2016

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
Copy link
Collaborator

@annevk annevk commented Apr 19, 2016

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

@jan-ivar
Copy link
Contributor

@jan-ivar jan-ivar commented Apr 19, 2016

@RByers unknown enum args in methods throw.

@tabatkins
Copy link
Contributor

@tabatkins tabatkins commented Apr 19, 2016

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
Copy link
Collaborator

@annevk annevk commented Apr 20, 2016

@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
Copy link
Author

@RByers RByers commented Apr 20, 2016

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
Copy link
Contributor

@tabatkins tabatkins commented Apr 20, 2016

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: #107 (comment) 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
Copy link
Contributor

@ddorwin ddorwin commented Apr 26, 2016

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?

@domenic
Copy link
Collaborator

@domenic domenic commented Jun 25, 2018

The OP contains the current way to do this.

@mgiuca
Copy link

@mgiuca mgiuca commented Sep 27, 2018

Where are we at with this? This came up in w3c/web-share#78 (actually the original context, adding a canShare method for detection of new features). We could go ahead and add canShare, but this discussion promises a more general method. It would be a shame if we standardized canShare and then this happened.

So is this something that's likely to happen? Could we use Web Share as a test subject?

@annevk
Copy link
Collaborator

@annevk annevk commented Sep 28, 2018

Why can't you use the method described in OP?

@bzbarsky
Copy link
Collaborator

@bzbarsky bzbarsky commented Sep 28, 2018

My take on that is that:

  1. That method is very non-obvious and people can easily mess it up.
  2. That method relies on being able to call the relevant API in a side-effect-free way (whether due to the API steps or due to ensuring the API throws), which may not always be possible.
@annevk
Copy link
Collaborator

@annevk annevk commented Sep 28, 2018

Why would 2 not be possible? The IDL layer can always throw, right? So throw something IDL would never throw, like 1. It seems hard for that to have a side-effect since at that point the specification algorithm hasn't run.

And although 1 is non-obvious, it seems better than maintaining a "supported features" side-table, which we've known historically to not work. (Perhaps we can do better now with better tests, but hmm...)

@bzbarsky
Copy link
Collaborator

@bzbarsky bzbarsky commented Sep 28, 2018

Why would 2 not be possible? The IDL layer can always throw, right? So throw something IDL would never throw, like 1

OK, but what happens in a UA that doesn't support that dictionary member?

@RByers
Copy link
Author

@RByers RByers commented Sep 28, 2018

@jan-ivar
Copy link
Contributor

@jan-ivar jan-ivar commented Sep 28, 2018

and ignore subtle bugs with old IE

And this is bad because...?

@jan-ivar
Copy link
Contributor

@jan-ivar jan-ivar commented Sep 28, 2018

OK, but what happens in a UA that doesn't support that dictionary member?

@bzbarsky Good point. Though in the case of share() at least, an invalid url should do it:

var supportsFiles = false;
try {
  await navigator.share(Object.defineProperty({url: ' '}, 'files', {get: function () {
    supportsFiles = true;
  }}));
} catch(e) {}
@bzbarsky
Copy link
Collaborator

@bzbarsky bzbarsky commented Sep 28, 2018

Right, for any specific API you can probably find a way. But it requires some creative thought every time, and is too easy to mess up.

Note that I am not a huge fan of the out-of-band list either. The only good news is that I think all browsers could code-generate that list from the same IDL that they use to code-generate their dictionary implementations. So apart from bugs in the code generators, some of the issues with the list not matching reality that we had in the past would probably be avoidable.

@tabatkins
Copy link
Contributor

@tabatkins tabatkins commented Sep 28, 2018

Yeah, OP's code is pretty insanely complex, and the fact that you actually have to custom-design it for every single feature (no generic pattern will work) makes it even worse, because it means you can't just abstract the test away into some library. We really do need to solve this generically via WebIDL.

@jan-ivar
Copy link
Contributor

@jan-ivar jan-ivar commented Sep 29, 2018

I feel there's a discrepancy between the 5 methods identified that need this, and the 400+ dictionaries in the platform.

Maybe it's not too much to ask of individual methods to think about this?

@tabatkins
Copy link
Contributor

@tabatkins tabatkins commented Oct 1, 2018

Those five cases aren't distinguished in any particular way - the need exists any time we add to a dictionary. Most dictionaries won't be expanded, true; they generally get defined once and then never touched again. But any dictionary could be expanded. Having to recognize which ones have been expanded, and figure out the unique key-testing method that each one has hopefully remembered to define for itself as soon as the first editor added to the dictionary, is not a good thing to foist on authors, and not a particularly dependable thing to require spec authors to remember to do.

If we're afraid of putting all the dictionary names on the global, we can always stuff it into a single method that takes a dictionary name and a key name, so there's no chance of future collisions.

@mgiuca
Copy link

@mgiuca mgiuca commented Oct 1, 2018

It honestly took me about 10 minutes to even understanding how that method works. To clarify, I assume it works by:

  1. Calling navigator.share with {url: ' '}, i.e., an invalid URL, which should according to the share spec reject with a TypeError.
  2. Injecting logic to determine whether a "files" attribute was accessed on the dictionary passed to share, before it threw a TypeError.

I can see quite a lot of problems with this approach:

  1. It assumes that the user agent's URL parser works correctly and rejects " ". While that should be the case, it means your Web Share feature detection is dependent on an edge case of the URL parser working correctly, which is notoriously non-compliant in all known user agents. [Note: In Chrome 69, this actually succeeds. I don't know why; we do fail more complex invalid URLs, but this one passes.]
  2. It assumes that the user agent invokes getters on a dictionary when a built-in function accesses its attributes. Again, this should be the case (?), but you're increasing the surface area for bugs in the user agent to mess with your feature detection.
  3. It assumes that the getter is invoked when the implementation merely checks for the presence of an attribute. (I don't know enough about defineProperty to know whether this is supposed to happen or not.) This feels implementation-dependent, depending on whether it gets the value of files, or merely uses something equivalent to hasOwnProperty to check for the existence of a "files" attribute.
  4. It assumes that the implementation checks the presence of the "files" attribute before checking whether the url is valid. The spec text says to reject if no known property is present, before checking the URL, but it seems brittle for a web developer to rely on the implementation doing those checks in the right order.
  5. It assumes that an implementation that reads the "files" attribute supports file sharing. That isn't necessarily true: I can imagine, for example, that we could gather metrics on how many developers are using "files" in their share dictionaries (thus triggering the getter) without actually supporting file sharing.
  6. It's extremely arcane. It's essentially impossible for a web developer to invent it by themself, and difficult to understand once you see it, so they just have to find it in documentation somewhere and copy+paste it, trusting that it works.

[Note: Other than the above issue where url: ' ' is accepted, if we change to a more complex invalid case: url: 'https://example.com:65536/', the above approach does work with navigator.share on Chrome 69 for Android.]

The above issues are specific to share, but they illustrate that this approach may have similar issues when applied to other APIs, or not even be possible (imagine if we hadn't invoked the URL parser, then the trick wouldn't even work).

I would rather provide an explicit API to detect whether things are supported, than the above approach which amounts to developers relying on observing "implementation details" (whether those are user-agent-specific details or just the quirks of the spec text) to find out whether it's supported. In my mind, the question is: should we go ahead and make a Web-Share-specific feature detection API, or should we scope out a general one that applies to any dictionary type?

@bzbarsky
Copy link
Collaborator

@bzbarsky bzbarsky commented Oct 1, 2018

It assumes that the user agent invokes getters on a dictionary when a built-in function accesses its attributes.

This is a very clear requirement in the Web IDL spec, there are tests for it using several dictionary types, and I am pretty sure all browsers get this right. There really shouldn't be failures here.

It assumes that the getter is invoked when the implementation merely checks for the presence of an attribute.

No, it assumes that the getter is invoked as part of Web IDL argument processing. Which, again, is a basic Web IDL spec requirement.

It assumes that the implementation checks the presence of the "files" attribute before checking whether the url is valid.

No. The presence check and the URL check both happen in the actual algorithm for the method, which runs after all the Web IDL argument processing is complete. There is no assumption being made here other than correct implementation of Web IDL method calls in general.

In general, all of points 2, 3, 4 come down to "a UA could have totally broken Web IDL argument handling", which I suspect is strictly less likely than "a UA could have its out-of-band support claims not match its actual support".

I can imagine, for example, that we could gather metrics on how many developers are using "files" in their share dictionaries (thus triggering the getter) without actually supporting file sharing.

This one is a bit odd, actually. Adding support for properties in dictionaries in observable ways (including calling getters) without actually supporting those properties is something that generally gets frowned on.

For purposes of the sort of telemetry you describe, I would personally implement it in terms of internal engine APIs that promised to not have side effects (e.g. not calling getters or proxy handlers). It would undercount uses that relied on such constructs, but I would not expect web developers to typically use those in dictionaries anyway.

I do agree with your points 1 and 6, which basically come down to my two concerns above: it's hard to make sure you call the API in a side-effect-free way and the whole thing is highly non-obvious. The big question for some people is whether those are enough to outweigh the extra likelihood of broken out-of-band support claims.

The above issues are specific to share

Only issue 1 is really specific to share in its specific phrasing.

@mgiuca
Copy link

@mgiuca mgiuca commented Oct 1, 2018

OK thanks @bzbarsky for explaining those. I now understand that the call to the getter happens during the IDL processing (converting a JavaScript Object to a ShareData value), before any of the Web Share algorithm steps take place, and thus the order of the steps is irrelevant. Therefore, my points 2–5 are invalid, and this is actually a much better reflection of whether the files attribute exists in the implementation's IDL dictionary than I had previously thought.

I still stand by points 1 and 6. This is still relying on URL processing to reject before share has any side effects.

@jan-ivar
Copy link
Contributor

@jan-ivar jan-ivar commented Oct 16, 2018

The reason I hesitate, is I don't consider a dictionary to be a first class API.

Someone once told me, "you don't have to use dictionaries. Use object. Problem solved." Their point being (I think) was that WebIDL defines dictionaries to encourage certain API patterns and discourage others. Therefore, it's not always sufficient to have a problem. The question is: is it a good pattern? For example, is share() really several methods in one? Might a discrete shareFile() method have been cleaner? I'm not saying it is, just using it as an example.

@mgiuca
Copy link

@mgiuca mgiuca commented Oct 17, 2018

For example, is share() really several methods in one? Might a discrete shareFile() method have been cleaner? I'm not saying it is, just using it as an example.

That decision isn't fixed in stone yet. It's part of this proposal:
http://wicg.github.io/web-share/level-2/

Maybe, but we also want to be able to share a file along with the other metadata at the same time (e.g., share a URL along with a screenshot).

What I'm asking here, at a high level, is: should there be a standardized pattern for how to do this kind of sub-feature detection, or should each API build its own? The suggestion to use a separate shareFile method suggests that each API should be finding its own way of supplying the new features.

@cben
Copy link

@cben cben commented Feb 10, 2019

Not sure if this was proposed yet, but how about stuffing the supported options onto the functions/methods that take them?
If I want to know how to call someElem.addEventListener(...), I'll probe someElem.addEventListener.supportedOptions or someElem.addEventListener.AddEventListenerOptions or something like that.
Minimal namespace pollution, and a pretty obvious place to look?

(For dictionaries taken by multiple functions, or methods that exist on multiple objects, such as .addEventListener, the specific standard should probably promise that the supported options are same for all places. Or not.)

@yoichio
Copy link

@yoichio yoichio commented May 11, 2020

I'm working with a customer who wants this kind of detection (with an origin trial, FYI).
I think this problem is not only for dictionary but function signature detection like "if this function supports boolean as the 3rd parameter?".
This reminds me System.Reflection on .Net C#.
I know they manages this kind of detection because of C# is strongly typed, but can WebIDL support this?

@npm1
Copy link

@npm1 npm1 commented Sep 28, 2020

Has there been any progress on this? We recently discussed w3c/performance-timeline#176 in a call and someone pointed out this issue, which would fix the problem for us.

@yoavweiss
Copy link

@yoavweiss yoavweiss commented Sep 29, 2020

In an unrelated thread, @mkruisselbrink points out that it's possible to feature detect dictionary members by providing the method an object with a getter and seeing if it was read.
That's cumbersome, but could work...

@RByers
Copy link
Author

@RByers RByers commented Sep 29, 2020

Yep, that's the pattern I used when opening this issue. We continue to hear developers complain that they really don't consider that acceptable (complexity, obscurity, desire to reliably avoid exceptions, etc).

@yoavweiss
Copy link

@yoavweiss yoavweiss commented Oct 1, 2020

Apologies for not actually reading the OP...
Agree that a simpler method would be better.

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

Successfully merging a pull request may close this issue.

None yet