Skip to content
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

Merged
merged 12 commits into from Jan 20, 2020

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

README.md Outdated Show resolved Hide resolved
lib/constructs/interface.js Outdated Show resolved Hide resolved
lib/transformer.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
test/cases/CEReactions.webidl Outdated Show resolved Hide resolved
test/__snapshots__/test.js.snap Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@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.

test/__snapshots__/test.js.snap Outdated Show resolved Hide resolved
test/__snapshots__/test.js.snap Outdated Show resolved Hide resolved
test/__snapshots__/test.js.snap Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
@TimothyGu
Copy link
Member

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

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

lib/constructs/interface.js Outdated Show resolved Hide resolved
lib/constructs/interface.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@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.

None yet

4 participants