jsdom / webidl2js

Auto-generate JS class structures for Web IDL specifications
MIT License
79 stars 30 forks source link

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

Open cdoublev opened 2 years ago

cdoublev commented 2 years ago

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.
domenic commented 2 years ago

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?

cdoublev commented 2 years ago

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);

domenic commented 2 years ago

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?

cdoublev commented 2 years ago

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 commented 2 years ago

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.

domenic commented 2 years ago

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?