webidl2js icon indicating copy to clipboard operation
webidl2js copied to clipboard

Add doc note about conversion of a value nested in a promise

Open cdoublev opened this issue 3 years ago • 6 comments

However, note that apart from Web IDL container return values, this impl-back-to-wrapper conversion doesn't work in a "deep" fashion. That is, if you directly return an impl, or return an array or object containing impls from a sequence<>, FrozenArray<>, or record<>-returning operation, the conversion will work.

Because a <Promise> is defined in Web IDL but there is no way to represent a promise value in IDL, a return value defined with Web IDL and wrapped in a promise will not be converted when the promise resolves, therefore I suggest the following modification:

- However, note that apart from Web IDL container return values, this impl-back-to-wrapper conversion doesn't work in a "deep" fashion.
+ However, note that apart from container return values that are defined with the Web IDL, this impl-back-to-wrapper conversion doesn't work in a "deep" fashion.

cdoublev avatar Jan 27 '22 10:01 cdoublev

I'm not sure what you mean by "there is no way to represent a promise value in IDL", or "a return value defined with Web IDL and wrapped in a promise will not be converted when the promise resolve". Could you give a code example?

domenic avatar Jan 27 '22 17:01 domenic

This is the description used in the Web IDL specification. I naively expected that an instance of a class defined with IDL and wrapped in a promise, as a return value from a class method also defined with Web IDL, could be returned as an instance of its wrapper class, due to what I remembered from the related section in the webidl2js documentation, and from rereading it again after I realized that it is not converted.

My suggestion is just an attempt to clarify that despite the presence of the Promise type in the web IDL specification, an instance wrapped in a promise can not be converted to an instance of the wrapper class.

The related Web IDL (CSSStyleSheet.replace()): Promise<CSSStyleSheet> replace(USVString text);

cdoublev avatar Jan 27 '22 18:01 cdoublev

This is the description used in the Web IDL specification.

This means there's no syntax for producing promise values, like 1 is syntax for producing a numeric value.

I naively expected

Sorry, this is quite hard to follow for me. Could you give a code example of what you wrote in the impl class, what you expected to happen, and what happened instead?

domenic avatar Jan 27 '22 18:01 domenic

It's my fault, I'm in a rush to finish this asap.

Web IDL:

interface CSSStyleSheet {
  Promise<CSSStyleSheet> replace();
};

Implementation class:

class CSSStyleSheetImpl {
  replace() {
    /* not meaningfull */
    return Promise.resolve().then(() => {
      /* not meaningfull */
      return this
      /* Workaround: return wrapperForImpl(this) */
    })
  }
}
module.exports = { implementation: CSSStyleSheetImpl }

(Failing) test file:

const CSSStyleSheetWrapper = require('wrappers/CSSStyleSheet.js')
describe('CSSStyleSheet.replace()', () => {
  it('should return the instance of CSSStyleSheet', async () => {
    const styleSheet = CSSStyleSheetWrapper.create(globalThis)
    const styleSheetRef = await styleSheet.replace()
    expect(styleSheetRef).toBe(styleSheet) // Fails (but expected to succeed)
    expect(CSSStyleSheetWrapper.is(styleSheetRef)).toBeTruthy() // Fails (but expected to succeed)
    expect(CSSStyleSheetWrapper.isImpl(styleSheetRef)).toBeTruthy() // Success (but not expected)
  })
})

cdoublev avatar Jan 27 '22 20:01 cdoublev

Here is a reproduction repo for what I'm trying to demonstrate (run node test.js). I also edited my last comment with more context on the "real" case for my issue.

I tried to understand why this is not converted but I have to admit that I don't even understand how the method of the wrapper instance can return a promise.

cdoublev avatar Feb 01 '22 14:02 cdoublev

Got it, yes, I can see how the docs aren't sufficiently clear here.

In fact, looking further it seems like they might just be wrong? I don't see any code for us to handle the sequence<>, FrozenArray<>, or record<> cases that the current README mentions. (We definitely don't handle the Promise<> case, except to convert any thrown exceptions into rejections.)

The ideal fix for this would be finishing up https://github.com/jsdom/webidl2js/pull/108.

In the meantime, the way we work around this is to write slightly-more-awkward impl classes, which use wrapperForImpl. E.g. here is an example of manually wrapping a sequence<>. I think that would work for your case?

domenic avatar Feb 01 '22 21:02 domenic