Ensure promise-typed operations and attributes return promises #200
Conversation
|
I found whatwg/webidl#776 which indicates we don't need to worry about supporting overloads between promise-returning and non-promise-returning operations. |
test/__snapshots__/test.js.snap
Outdated
| throw new TypeError(\\"Illegal invocation\\"); | ||
| } | ||
|
|
||
| return utils.tryWrapperForImpl(esValue[implSymbol].promiseOperation()); |
There was a problem hiding this comment.
This has to generate:
return utils.tryWrapperForImpl(await esValue[implSymbol].promiseOperation());There was a problem hiding this comment.
This and your other unsolicited comments are not so straightforward.
You are suggesting that we incorporate the unwrapping done in #108 here. However, that breaks the ability for the impl method/getter to return the same promise multiple times. That is especially problematic for attributes, where x.attr === x.attr is almost always what the spec intends.
I think we should leave working on the unwrapping to #108 or some continuation of that has better discussion behind it.
test/__snapshots__/test.js.snap
Outdated
| throw new TypeError(\\"Illegal invocation\\"); | ||
| } | ||
|
|
||
| return utils.tryWrapperForImpl(esValue[implSymbol][\\"promiseAttribute\\"]); |
There was a problem hiding this comment.
This has to generate:
return Promise.resolve(esValue[implSymbol]["promiseAttribute"])
.then(utils.tryWrapperForImpl);
test/__snapshots__/test.js.snap
Outdated
| } | ||
|
|
||
| static async staticPromiseOperation() { | ||
| return utils.tryWrapperForImpl(Impl.implementation.staticPromiseOperation()); |
There was a problem hiding this comment.
This has to generate:
return utils.tryWrapperForImpl(await Impl.implementation.staticPromiseOperation());
test/__snapshots__/test.js.snap
Outdated
| try { | ||
| const esValue = this !== null && this !== undefined ? this : globalObject; | ||
|
|
||
| return Impl.implementation[\\"staticPromiseAttribute\\"]; |
There was a problem hiding this comment.
This has to generate:
return Promise.resolve(Impl.implementation["staticPromiseAttribute"])
.then(utils.tryWrapperForImpl);
|
Please do not review pull requests that your review was not requested on. It is very rude to come into someone else's project and critique their code unsolicited. |
| @@ -25,6 +25,20 @@ class Operation { | |||
| return firstOverloadOnInstance; | |||
| } | |||
|
|
|||
| isAsync() { | |||
| const firstAsync = this.idls[0].idlType.generic === "Promise"; | |||
There was a problem hiding this comment.
Linking to whatwg/webidl#776 would be useful.
lib/constructs/interface.js
Outdated
| this.str += ` | ||
| ${name}(${formatArgs(args)}) {${body}} | ||
| ${isStatic ? "static " : ""}${isAsync ? "async " : ""}${name}(${formatArgs(args)}) {${body}} |
There was a problem hiding this comment.
Do we actually need the function to be async? I don't see await being used anywhere, and I fear that this difference could in fact be observable by the number of ticks it passed before the promise is rejected with a synchronously thrown error.
There was a problem hiding this comment.
I talk about that in #79 (comment).
There was a problem hiding this comment.
It's a convenient way to convert exceptions into rejections. But, it sounds like folks are in favor of instead wrapping everything in try/catch, so I'll do that.
domenic
force-pushed
the
promise-returning
branch
from
d521dc9
to
6c4bea9
Compare
domenic
force-pushed
the
promise-returning
branch
from
6c4bea9
to
ecd4db4
Compare
Closes #79. This removes support for overloading promise operations and non-promise operations, since that would require a more ambitious approach and doesn't seem to appear on the web.
domenic
force-pushed
the
promise-returning
branch
from
ecd4db4
to
8606adc
Compare
test/cases/PromiseTypes.webidl
Outdated
| [Unforgeable] Promise<void> unforgeablePromiseOperation(); | ||
| [Unforgeable] readonly attribute Promise<void> unforgeablePromiseAttribute; |
There was a problem hiding this comment.
| [Unforgeable] Promise<void> unforgeablePromiseOperation(); | |
| [Unforgeable] readonly attribute Promise<void> unforgeablePromiseAttribute; | |
| [LegacyUnforgeable] Promise<void> unforgeablePromiseOperation(); | |
| [LegacyUnforgeable] readonly attribute Promise<void> unforgeablePromiseAttribute; |
Closes #79.
This removes support for overloading promise operations and non-promise operations, since that would require a more ambitious approach and doesn't seem to appear on the web.
Thoughts welcome. The strategy of using
asyncfor operations and try/catch for attributes makes the output nice but the webidl2js code a bit icky. Threading the async bit through all the operation stuff adds a bit of complexity. And, we might want to abandon that if we do want to support overloads of promise-returning operations vs. non-promise returning ones, since doing that would require an appropriately-scoped try/catch.