jsdom / webidl2js

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

Remove undocumented Impl.init functionality #195

Closed domenic closed 4 years ago

domenic commented 4 years ago

This is not used by jsdom and seems unnecessary.

domenic commented 4 years ago

I've confirmed that this works on jsdom just as well as the current master does. (The current master seems to have problems but I think they are solved by https://github.com/jsdom/jsdom/pull/2884.)

domenic commented 4 years ago

It seems DOMException's usage could be replaced by moving that code into the constructor, do you agree?

exports.implementation = class DOMExceptionImpl {
  constructor(globalObject, [message, name], { wrapper }) {
    this.name = name;
    this.message = message;

    if (Error.captureStackTrace) {
      Error.captureStackTrace(wrapper, wrapper.constructor);
    }
  }

  get code() {
    return legacyErrorCodes[this.name] || 0;
  }
};
ExE-Boss commented 4 years ago

That still won’t support SpiderMonkey’s Error.prototype.stack getter, which requires the wrapper to be created using Reflect.construct(Error, args, ctor).

domenic commented 4 years ago

I don't understand the relevance of that comment. SpiderMonkey is not supported today and the wrapper is not created in that way.

TimothyGu commented 4 years ago

I think the new method should also create the stack property. Moving it into the impl constructor would mean that the stack property is only added when create/createImpl is called.

domenic commented 4 years ago

Hmm, I think you're right. I wish there were a way to do that without adding three lines to every generated output file (possibly 6 if I can't find a good refactoring).

The only thing I can think of is a custom extended attribute, e.g. [WebIDL2JSHasExtraInit] interface DOMException { ... }, which is not very fun either.

I guess we can just wait until we get optional chaining syntax and raise the minimum V8 version that high, to bring it down to 1 line.