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 [CEReactions] and [HTMLConstructor] processors #130
Conversation
Thanks so much for the review @TimothyGu. I don't have much to add, just that overall I'm really happy with this approach and excited about what it represents (i.e., progress on custom elements in jsdom). |
@TimothyGu Ready for another pass of review. |
@pmdartus I pushed some doc tweaks; let me know what you think. I would also like to make the HTMLConstructor example slightly more realistic (like I did with the try/finally for CEReactions); if you have any ideas there, or samples of the jsdom code you're planning on uploading, let me know. Do you think we should do #131 before or after landing this PR? My instinct is to try to finish a first draft of custom element support ASAP, so not wait on #131. But if you want to do that then I'll do my best to be speedy about reviews, now that my vacation is over :). |
Oh, I see that jsdom/jsdom#2548 is updated to use this already. I'll tweak the documentation to reflect how things work for [HTMLConstructor]. And then I'll try to work on reviewing/pushing tweaks for that! I think it'll be best to merge this when the jsdom PR is approved, just in case we want to make any extra changes here as part of the review process over there. |
36d6946
to
251a03c
Compare
@pmdartus Looks like this now requires some additional rebasing. Feel free to merge otherwise. |
cfcf811
to
418a9cf
Compare
@domenic @TimothyGu I made some last-minute changes to the processor interface and updated the documentation. It would be great if you can give a look at it one last time before I merge it. |
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.
Some last minute comments. LGTM otherwise.
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.
Imports aren’t currently being de‑duplicated, so you have to manually avoid creating duplicate imports.
2ef92f4
to
cb05c96
Compare
42720bc
to
14ad05a
Compare
Motivation
In order to implement Custom Element in jsdom, we need to add support for
[CEReactions]
and[HTMLConstructor]
IDL extended attributes. Since those 2 extended attributes require some knowledge about jsdom implementation details, we need a way for JSDOM to hook itself into the code generation.Changes
This PR introduces new 2 optional processor
processCEReactions
andprocessHTMLConstructor
. Those processors receive 2 arguments: the parsed IDL property by webidl2 and the code generated by webidl2js, and expect to return some post-processed code.Here is an example of how those new hooks would integrate into jsdom: convert.js
Note that this PR is already out of date because of the introduction of the new WebIDL
constructor
syntax. I opened another issue for theconstructor
syntax upgrade (#131).