-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Implement callback interfaces #178
Conversation
The code for generating the constants seems really over-complicated, but I guess it is basically a copy and paste of the code for interfaces. I spent five minutes trying to simplify but it got kind of messy, so I won't pursue that for now. If people have ideas though, I'd be interested. |
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.
There is no such thing as Reflect.call
in ECMAScript.
There is only Reflect.apply(…)
.
lib/types.js
Outdated
const resolvedTypes = new Set([ | ||
"callback interface", | ||
"dictionary", | ||
"enumeration", | ||
"interface" | ||
]); | ||
const resolvedTypes = new Set(["callback interface", "dictionary", "enumeration", "interface"]); |
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 made this multi‑line so that when there’s multiple in‑flight PRs that add new types (like #123), they avoid conflicting with each other.
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 don't think that's a sufficient motivation for making the code more verbose in that way. In general, please do not change project style without opening an issue for discussion.
|
||
const argsToReflectCall = argNames.length > 0 ? `, ${argNames.join(", ")}` : ""; | ||
this.str += ` | ||
let callResult = Reflect.call(X, thisArg${argsToReflectCall}); |
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.
There is no such thing as Reflect.call(…)
, so this line will throw a TypeError
.
I’ve fixed that in #172.
Closes #178 by superseding it. Co-authored-by: Domenic Denicola <[email protected]>
Needed for fixing jsdom/jsdom#2869 (used in jsdom/jsdom#2884).
Closes #172 by superceding it. I can actually edit this branch.