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

TAG Feedback: Should supportsSession resolve with a boolean? #824

Closed
toji opened this issue Sep 4, 2019 · 6 comments · Fixed by #841
Closed

TAG Feedback: Should supportsSession resolve with a boolean? #824

toji opened this issue Sep 4, 2019 · 6 comments · Fixed by #841
Labels
potential breaking change Issues that may affect the core design structure.

Comments

@toji
Copy link
Member

toji commented Sep 4, 2019

From the TAG feedback for the WebXR API:

Why does navigator.xr.supportsSession() not return a Promise<boolean> rather than rejecting in the case that the session type is not supported? That would seem like a better semantic match to the wording of the method, as well as not requiring the author to program a simple feature detection step in a try/catch style.

(A brief technical tangent: This method is asynchronous because we expect that in many cases this call will be the first indication a page has that XR capabilities are desired, and initializing the resources required to answer whether or not a given mode is supported is likely heavyweight enough that it's appropriate to not block the main thread waiting for an answer.)

The current behavior (rejecting when an unsupported mode is passed) was discussed with @bfgeek to review ergonomics a while back, but it's probably worth briefly revisiting to ensure that we haven't broken any assumptions about the functionality since then.

It's worth noting that several other bits of API surface (requestSession, requestReferenceSpace, etc.) use this same style, where rejecting the promise indicates failure, and we likely want to remain consistent in how we interact with promises throughout the API. Also worth noting that, as far as I can see, the current behavior is consistent with the use of other Promise<void>s in APIs such as WebUSB and WebAudio.

@toji toji added the editor triage Issues marked for triage/re-examination by the editors label Sep 4, 2019
@domenic
Copy link

domenic commented Sep 5, 2019

Strong +1 to changing this. (And potentially changing other methods, but not necessarily.) Remember, rejections are for exceptional situations. Would you throw a sync exception from a function named "supportsX" to indicate that you did not support something? See https://www.w3.org/2001/tag/doc/promises-guide#rejections-should-be-exceptional

@toji toji added this to the September 2019 milestone Sep 5, 2019
@toji
Copy link
Member Author

toji commented Sep 5, 2019

Thanks for that feedback, Domenic! If you don't mind my asking a followup question:

We have other Promise-based APIs that return a resource with certain properties if and only if the platform supports it. For example: navigator.xr.requestSession(). This follows the pattern that we observed in other APIs such as navigator.usb.requestDevice() which rejects if a device fulfilling the given requirements cannot be found.

It's not clear if lack of hardware support is considered an "exceptional situation" that would merit an exception/reject. It feels like it could conceivable fall under the "When a value is asked for asynchronously and is not found" guidance in the doc you linked, but it also feels like it may fall under the "APIs that are more ambiguous about being a question versus a demand" guidance as well. What we did consider is that there are several unambiguously exceptional reasons which might cause the promise to reject, such as feature policy not allowing the call or the user denying consent to access the hardware. It felt strange to say that the method could report failure in two distinct ways, both of which the developer would have to handle separately in a well behaved application.

@domenic
Copy link

domenic commented Sep 5, 2019

I guess I'll answer your question with another. Let's say those methods were synchronous. How would you signal the different type of failures? Would you signal them all with exceptions?

In general, the thing about exceptions---or promise rejections---is that they don't have to be handled immediately; instead they bubble upward toward the nearest catch block. That is, you can group multiple exceptional failures into a single failure handler, perhaps one for your entire app. Whereas non-exceptional failure cases need to be handled immediately, and the API design encourages this by using return values which must be checked before they are used.

So for example, it seems like a hardware failure is clearly not something that needs to be handled immediately; that could be bubble up to a generic "something went wrong". Whereas "session type is not supported" seems like it should probably be handled more immediately, because after all, the web developer is asking for whether something is true or not, and they presumably want to know the answer in the calling code; they don't want lack of support to bubble up to some generic failure handler.

I hope this is helpful; it's hard for me to judge specifics of a given API, and indeed there may be some gray area. But again, I'll just ask you, if you were writing in a language where such APIs were synchronous (perhaps C++), how would failures be communicated? Exceptions, return values, or a mixture of both? That's always the best guide.

@toji toji added agenda Request discussion in the next telecon/FTF and removed editor triage Issues marked for triage/re-examination by the editors labels Sep 9, 2019
@AdaRoseCannon
Copy link
Member

/facetoface There was a poll in today's call, discuss with TAG on face 2 face.

@toji toji removed the agenda Request discussion in the next telecon/FTF label Sep 10, 2019
@toji
Copy link
Member Author

toji commented Sep 10, 2019

To recap the discussion today:

After outlining the above discussions it was brought up by Mounir that this would break any existing content, though there's relatively little of it so far. Sam indicated that Chrome's origin trial has ~100 developers signed up. I pointed out that while any content developed under an Origin Trial would already have backwards compatibility broken, for this feature it would break in a way that developers might easily miss, since supportSession failures would still result in a resolved promise. Also would be too late to merge into Chrome 78.

Kip mentioned that there were indeed other APIs following the convention of returning a boolean, but they were relatively sparse. Also relayed unofficial guidance from individuals at Mozilla that said "If the method doesn't return the thing you asked for, it should reject." Meaning that something like requestSession would reject on failure because it didn't give you a session but supportsSession would still resolve in the negative case because it was giving you an answer to the implicit question asked when you called it. (I personally like that mental model).

Rik suggested having an alternative entry point and deprecating supportsSession. I expressed that I'm not personally willing to advocate for shipping an API with a deprecated method in it from day one, but I do support the idea that if the return behavior changes we should change the method signature in such a way that it would clearly indicate that the old method was no longer supported.

Loc Dao mentioned that they had 4-5 WebXR projects in progress and that these kind of changes took non-trivial effort to keep up with. Supported the change long term but hoped that it wouldn't be a regular occurrence.

Did a straw poll on if call attendees were in favor of the change. Result was roughly an even split. Will revisit at TPAC, preferably with the TAG in the room.

@toji toji added the potential breaking change Issues that may affect the core design structure. label Sep 11, 2019
toji added a commit that referenced this issue Sep 11, 2019
Fixes #824. Acts on feedback from the TAG that supportsSession should
resolve with a boolean rather than reject when a session mode is not
supported. In order to aid developers the method name was changed so
that previous uses of the old method will be clearly identified as
failing, rather than silently report that a session was supported that
shouldn't be because the app failed to check the resolved boolean value.
@toji
Copy link
Member Author

toji commented Sep 13, 2019

Two updates:

  1. A related issue (mentioning this API directly) has been opened on the TAGs design principles repo. Best practices for feature detection of DOM API w3ctag/design-principles#137
  2. I mentioned in my previous comment that the straw poll taken on the WG call indicated an even split of opinion for and against the change. This is true in terms of the individuals that responded, but when broken down by member organization the result was actually Google disagreeing with the change and every other member expressing support. To ensure there was no confusion in the results a call for consensus was put out on the mailing list: https://lists.w3.org/Archives/Public/public-immersive-web-wg/2019Sep/0007.html

toji added a commit that referenced this issue Sep 19, 2019
Fixes #824. Acts on feedback from the TAG that supportsSession should
resolve with a boolean rather than reject when a session mode is not
supported. In order to aid developers the method name was changed so
that previous uses of the old method will be clearly identified as
failing, rather than silently report that a session was supported that
shouldn't be because the app failed to check the resolved boolean value.
@toji toji closed this as completed in #841 Sep 26, 2019
toji added a commit that referenced this issue Sep 26, 2019
Fixes #824. Acts on feedback from the TAG that supportsSession should
resolve with a boolean rather than reject when a session mode is not
supported. In order to aid developers the method name was changed so
that previous uses of the old method will be clearly identified as
failing, rather than silently report that a session was supported that
shouldn't be because the app failed to check the resolved boolean value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential breaking change Issues that may affect the core design structure.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants