-
-
Notifications
You must be signed in to change notification settings - Fork 29
Create exceptions and promises in the correct realm #251
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
Conversation
Closes #242 by superseding it.
c9909c2
to
1f99bb2
Compare
Since 9260316 jsdom has not worked, seemingly due to some weird interaction between Symbol.for and VM context objects. This seems to fix things, and it's not clear why we used a cross-realm symbol for this anyway.
Review appreciated if either of you have time, @TimothyGu @ExE-Boss. 8faa068 in particular I am unsure what is going on. |
lib/output/utils.js
Outdated
|
||
// This only contains the intrinsic names that are referenced from the `ctorRegistry`: | ||
const intrinsicConstructors = ["Array"]; | ||
const ctorRegistrySymbol = Symbol("[webidl2js] constructor registry"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used a global symbol in #140 so that the constructor registry would be shared between jsdom
, domexception
, whatwg‑url
, and eventually cssstyle
(once jsdom/cssstyle#116 is merged).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. Probably then the problem is that I was testing only upgrading webidl2js for jsdom, but not domexception and whatwg-url. I will have to test upgrading all three. Or maybe it would be best to add a major version number to the symbol description so that if we do mix versions it doesn't cause problems.
const ctorRegistrySymbol = Symbol.for("[webidl2js] constructor registry"); | ||
|
||
// This only contains the intrinsic names that are referenced from the `ctorRegistry`: | ||
const intrinsicConstructors = ["Array"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing these in the constructor registry was done to correctly implement the parts of WebIDL that use intrinsic objects, and to allow using extends ctorRegistry.<base‑name>
instead of extends globalObject.<base‑name>
, the former of which also requires that all code using WebIDL2JS uses the same ctorRegistry
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still implement the parts Web IDL that use intrinsic objects correctly. And I don't see the value of extends ctorRegistry.
instead of extend globalObject.
; indeed requiring everything to use the same ctorRegistry seems subpar instead of using the global as the coordination point.
Closes #242 by superseding it.
This approach moves the ctorRegistry back to being mostly for the registration of WebIDL2JS-generated constructors. We use the globalObject variable to pass to webidl-conversions and to find our Promise and TypeError constructors.
Needs testing with jsdom before merging.