Skip to content

Conversation

@pmdartus
Copy link
Member

@pmdartus pmdartus commented Sep 25, 2019

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 and processHTMLConstructor. 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 the constructor syntax upgrade (#131).

@domenic
Copy link
Member

domenic commented Sep 26, 2019

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).

@pmdartus
Copy link
Member Author

@TimothyGu Ready for another pass of review.

@domenic
Copy link
Member

domenic commented Oct 13, 2019

@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 :).

@pmdartus
Copy link
Member Author

Thanks for fixing my broken fr-english, the documentation looks way better now!

I think you are right, I would prefer to release a first draft of CE and get some initial feedback before tackling #131. In terms of ordering, I was thinking about working on #131 once #138 is merged.

@domenic
Copy link
Member

domenic commented Oct 13, 2019

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.

@pmdartus pmdartus force-pushed the pmdartus/add-processor branch from 36d6946 to 251a03c Compare November 26, 2019 06:49
@TimothyGu
Copy link
Member

@pmdartus Looks like this now requires some additional rebasing. Feel free to merge otherwise.

@pmdartus pmdartus force-pushed the pmdartus/add-processor branch from cfcf811 to 418a9cf Compare December 31, 2019 08:45
@pmdartus pmdartus requested review from TimothyGu and domenic January 2, 2020 15:28
@pmdartus
Copy link
Member Author

pmdartus commented Jan 2, 2020

@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.

Copy link
Member

@TimothyGu TimothyGu left a 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.

Copy link
Contributor

@ExE-Boss ExE-Boss left a 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.

@TimothyGu TimothyGu force-pushed the pmdartus/add-processor branch from 2ef92f4 to cb05c96 Compare January 4, 2020 08:24
@TimothyGu
Copy link
Member

Rebased and added a bit of documentation change as well as @ExE-Boss's comments. @domenic I'll let you do the honors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants