-
-
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
Move reflection support to host-side using processors #161
Conversation
lib/context.js
Outdated
@@ -75,10 +81,22 @@ class Context { | |||
const context = { | |||
addImport(source, imported) { | |||
return requires.add(source, imported); | |||
}, | |||
addAbsImport(source, imported) { |
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.
This turns out to be necessary for addImport("whatwg-url")
.
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’d prefer if addImport(…)
supported package imports, which is what I’m doing in #162.
Filed w3c/webidl2.js#455 to account for the desire for more flexible extended attributes. |
It has long been proposed that reflection features be moved to the host-side, considering it is a part of HTML rather than Web IDL. With the recently added processor infrastructure, this is now possible. With this change, non-relative imports are now allowed in processors' addImport function. This is done by checking if the imported package fits the npm package name regular expression. Thanks to ExE Boss for this contribution. We also note that existing reflectors did not deal very well with the change from this to esValue, since 2a2a099. Since this commit moves the reflection code out of webidl2js, the bug now naturally disappears. Closes jsdom#15. Closes jsdom#93. Co-authored-by: ExE Boss <[email protected]>
Okay, this has been rebased and updated in the following ways:
I'll update jsdom/jsdom#2813 as well soon. PTAL. |
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.
It seems like this is enough to replace our current jsdom reflection support, and improve it by adding ReflectURL. It isn't clear it allows full implementation of the plan in whatwg/html#3238 (comment) (in particular the use of strings and other non-identifier RHSs), but I have no problem proceeding with this to start.
Co-Authored-By: Domenic Denicola <[email protected]>
Co-Authored-By: Domenic Denicola <[email protected]>
Co-Authored-By: Domenic Denicola <[email protected]>
Add proper reflection support through processor
It has long been proposed that reflection features be moved to the host-side, considering it is a part of HTML rather than Web IDL. With the recently added processor infrastructure, this is now possible.
With this change, non-relative imports are now allowed in processors' addImport function. This is done by checking if the imported package fits the npm package name regular expression. Thanks to @ExE-Boss for this contribution.
We also note that existing reflectors did not deal very well with the change from this to esValue, since 2a2a099. Since this commit moves the reflection code out of webidl2js, the bug now naturally disappears.
Closes #15. Closes #93.