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

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

Open
cdoublev opened this issue Jan 27, 2022 · 6 comments
Open

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

cdoublev opened this issue Jan 27, 2022 · 6 comments

Comments

@cdoublev
Copy link

cdoublev commented Jan 27, 2022

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

domenic commented Jan 27, 2022

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

cdoublev commented Jan 27, 2022

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

domenic commented Jan 27, 2022

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

cdoublev commented Jan 27, 2022

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

cdoublev commented Feb 1, 2022

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

domenic commented Feb 1, 2022

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 #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?

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

No branches or pull requests

2 participants