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

Rework of DOM shim (using parse5) to allow for element properties in the future #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

briangrider
Copy link

@briangrider briangrider commented Nov 21, 2024

Related Issue

First step to resolving issue 170

Summary of Changes

dom-shim.js:

  1. Rework DOM shim to use parse5 for serializing children and parsing strings for innerHTML getter and setter
  2. Get ShadowDom "closed" mode working
  3. Create declarative shadow DOM to mimic the browser functionality when content is appended to a shadowRoot
  4. Remove template auto-wrapping functionality from HTMLTemplateElement
  5. Add isShadowRoot utility function to check prototype when assessing whether shadowrootmode attribute needs to be added to appended template/content
  6. Add deepClone utility function to be used with cloneDeep(deep) functionality (this just returned the actual element before)
  7. Add getParse5ElementDefaults utility function to generate empty parse5 elements with much less overhead. At least 2-5x performance gain compared to parsing an empty element in a string (ie. '<template></template>')
  8. Add various "TODO" comments for considerations for improvement

wcc.js

  1. Simplify renderComponentRoots - no need for getInnerHTML, innerHTML, elementHTML, elementTree, hasLight anymore
  2. Remove renderLightDomChildren (passes all tests without)
  3. Remove VOID_ELEMENTS (no longer needed by renderLightDomChildren)
  4. Simplify renderToString, similar to renderComponentRoots

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try and give this a more thorough testing soon, but just a couple quick observations:

  • Since there are no dependency changes, package-lock.json should probably not be changing
  • It looks like the tests failed only because of coverage thresholds, and so I'm fine bumping down that as needed (in this case to 85% would be ok)

@briangrider
Copy link
Author

briangrider commented Nov 22, 2024

Will try and give this a more thorough testing soon, but just a couple quick observations:

  • Since there are no dependency changes, package-lock.json should probably not be changing

Ah, my bad. I must have had it accidentally checked on Sourcetree, just reset the branch and pushed just the relevant files

  • It looks like the tests failed only because of coverage thresholds, and so I'm fine bumping down that as needed (in this case to 85% would be ok)

Yes, I believe on my initial commit I was at around 91+% on function coverage but it looks like it dipped to 89.28% with some of the additional changes. Yes, if we can temporarily drop the threshold to 85% that would be great

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, coming out of our chats in #177, here's the state of things

  1. We will land feature/issue 177 Replaced deprecated method getInnerHTML with getHTML #171 first
  2. preserve getHTML in the DOM shim and make sure no other APIs are being removed (and we should probably add some test cases here, which I can help with)
  3. I am OK to let the mode fix come in as part of this PR, but let's make sure by adding a test case - support configurable shadowrootmode attribute for <template> tags #65
  4. Looks like this PR would also help resolve or invalidate verify / ensure proper serialization of shadow roots excluding closed shadow roots from getInnerHTML #16 ?
  5. Sanity test in Greenwood (I will help with this once this gets rebased after feature/issue 177 Replaced deprecated method getInnerHTML with getHTML #171 lands)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.16.0 bug Something isn't working enhancement Improvement to existing functionality
Projects
Status: 👀 In review
2 participants