enhancement/rework of DOM shim (using parse5) to allow for element properties in the future by briangrider · Pull Request #178 · ProjectEvergreen/wcc

briangrider · 2024-11-21T21:54:35Z

Related Issue

First step to resolving issue 170

Summary of Changes

dom-shim.js:

Rework DOM shim to use parse5 for serializing children and parsing strings for innerHTML getter and setter
Get ShadowDom "closed" mode working
Create declarative shadow DOM to mimic the browser functionality when content is appended to a shadowRoot
Remove template auto-wrapping functionality from HTMLTemplateElement
Add isShadowRoot utility function to check prototype when assessing whether shadowrootmode attribute needs to be added to appended template/content
Add deepClone utility function to be used with cloneDeep(deep) functionality (this just returned the actual element before)
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>')
Add various "TODO" comments for considerations for improvement

wcc.js

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

thescientist13

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 · 2024-11-22T23:41:09Z

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

…ntent - other relevant changes as well

thescientist13

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

We will land #171 first
rebase this branch against master
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)
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 - #65
Looks like this PR would also help resolve or invalidate #16 ?
Sanity test in Greenwood (I will help with this once this gets rebased after #171 lands)

Added test for create-document-fragment Other minor tweaks

# Conflicts: # src/dom-shim.js # src/wcc.js

briangrider · 2025-01-17T17:54:59Z

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)

I think we should be good here but let me know.

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

This seems to be the last step here on my end. Will have this done soon.

Looks like this PR would also help resolve or invalidate verify / ensure proper serialization of shadow roots excluding closed shadow roots from getInnerHTML #16 ?

Yes, I believe so.

thescientist13

Looking good!

Just left a few comment / questions, but let me go ahead and start doing some downstream testing of this in Greenwood in the meantime.

thescientist13 · 2025-01-17T18:57:22Z

+
+  before(async function () {
+    const { html } = await renderToString(new URL('./src/index.js', import.meta.url));
+    console.log('hello', html);


clean up console.log

thescientist13 · 2025-01-17T19:02:47Z

+ * Run wcc against two custom elements, one with an open shadow root and one with a closed shadow root
+ *
+ * User Result
+ * Should return the expected HTML output based on the shadow root mode of each component


Minor suggestion here to maybe get a little more technical / specific on the result here

Should return the expected attribute value for shadowrootmode on the template tag based on the shadow root mode of each component

thescientist13 · 2025-01-17T19:09:21Z

@@ -0,0 +1,61 @@
+/*
+ * Use Case
+ * Run wcc against a custom element which creates a document fragment


Sorry, I might have the lost recollection on this, but was there a particular need to test for document fragment specifically? Not sure if the case / result maybe just need to cover a little bit more of the why, maybe that would help jog my recollection?

thescientist13 · 2025-01-17T19:11:19Z

  }

  define(tagName, BaseClass) {
+    // TODO Should we throw an error here when a tagName is already defined?


do we need this comment? Seems its similar as the comment below?

thescientist13 · 2025-01-17T19:14:18Z

+
+import { parseFragment, serialize } from 'parse5';
+
+// TODO Should go into utils file?


re: utils file, if its not needed in other files, it's probably fine to leave. I think there are cases for a util file between wcc and jsx-loader I think, but I think for now we can probably just move the TODO comment, but certainly open to a PR that proposes some code structuring / organization.

thescientist13 · 2025-01-17T19:16:34Z

+}
+
+// Deep clone for cloneNode(deep) - TODO should this go into a utils file?
+// structuredClone doesn't work with functions. TODO This works with


Are you suggesting a library would / could handle functions? If we think that feature would be helpful, I would prefer we just extend this to support our needs if so. (would prefer to keep dependencies in this project as slim as possible and already watch to try and better de-couple the JSX ones).

So yeah, if we want to keep iterating on this, I am happy to have an issue / PR to do, and we can clean up the TODO comment.

thescientist13 · 2025-01-17T19:16:50Z

+}
+
+// Creates an empty parse5 element without the parse5 overhead. Results in 2-10x better performance.
+// TODO Should this go into a utils files?


same: re utils file comment

thescientist13

So did some downstream testing in Greenwood and I see one regression related to how WCC is handling <html>, <head> and <body> tags it seems, in that it looks like they are being stripped out by WCC now.

For example, this "app layout" component
https://github.com/ProjectEvergreen/greenwood/blob/master/packages/cli/test/cases/build.default.workspace-layouts-page-and-app-dynamic/src/layouts/app.js

export default class AppLayout extends HTMLElement {
  async connectedCallback() {
    const year = new Date().getFullYear();

    this.innerHTML = `
      <!DOCTYPE html>
      <html>
        <head>
          <title>App Layout</title>
        </head>

        <body>
          <h1>App Layout</h1>
          <page-outlet></page-outlet>
          <footer>${year}</footer>
        </body>
      </html>
    `;
  }
}

when run through WCC, doesn't seem to be returning all the content (notice the <title> tag is not wrapped by a <head> tag anymore)

{
  result: {
    layout: null,
    body: '\n' +
      '      \n' +
      '      \n' +
      '        \n' +
      '          <title>App Layout</title>\n' +
      '        \n' +
      '\n' +
      '        \n' +
      '          <h1>App Layout</h1>\n' +
      '          <page-outlet></page-outlet>\n' +
      '          <footer>2025</footer>\n' +
      '        \n' +
      '      \n' +
      '    ',
    frontmatter: null,
    html: null,
    prerender: false,
    isolation: undefined
  }
}

My hunch is that in DOM shim, now that we are pulling in parse5, we are only using parseFragment
https://github.com/ProjectEvergreen/wcc/pull/178/files#diff-d043dc2785231f2ef65d347d7810dae3ca0224e06f777e6d3d66ccc18c5132eeR3

but like in wcc.js we should probably be detecting and using parse or parseFragment
https://github.com/ProjectEvergreen/wcc/blob/master/src/wcc.js#L31

This may be an oversight in the WCC test case, so let's add a new one specifically testing for full document support and I bet it will fail, then we can add the parsing detection, then I bet it will work. 🎯

thescientist13 · 2025-01-19T23:09:29Z


  getTemplate() {
-    return `
+    return `<!DOCTYPE html>


since we have dedicated test cases for the wrapping tags now, maybe we can leave this test case as is?

thescientist13

Awesome, just sanity checked the latest changes against Greenwood's test suite (and website repo) and everything looks to be working great!
ProjectEvergreen/greenwood@1be1553

Only last thing I commented on was re: this test case, otherwise I think this is looking good to go. 💯

all regression tests passing

thescientist13

Heck yeah, awesome work @briangrider ! 🙇

briangrider · 2025-01-20T15:09:32Z

Heck yeah, awesome work @briangrider ! 🙇

Happy to help!

thescientist13 reviewed Nov 22, 2024

View reviewed changes

briangrider force-pushed the master branch from adfab2e to b11ad42 Compare November 22, 2024 23:34

briangrider closed this Nov 23, 2024

briangrider force-pushed the master branch from b11ad42 to 22078ee Compare November 23, 2024 00:51

updated dom shim to use child nodes instead of strings for element co…

9b921e2

…ntent - other relevant changes as well

briangrider reopened this Nov 23, 2024

thescientist13 mentioned this pull request Nov 23, 2024

Replace deprecated method getInnerHTML with getHTML #177

Closed

thescientist13 added enhancement Improvement to existing functionality 0.16.0 labels Dec 2, 2024

thescientist13 added this to the 1.0 milestone Dec 2, 2024

This was linked to issues Dec 2, 2024

support configurable shadowrootmode attribute for <template> tags #65

Closed

Issue Setting Element Properties #170

Closed

thescientist13 added the bug Something isn't working label Dec 2, 2024

thescientist13 assigned briangrider Dec 2, 2024

thescientist13 mentioned this pull request Dec 2, 2024

verify / ensure proper serialization of shadow roots excluding closed shadow roots from getInnerHTML #16

Closed

thescientist13 linked an issue Dec 2, 2024 that may be closed by this pull request

verify / ensure proper serialization of shadow roots excluding closed shadow roots from getInnerHTML #16

Closed

thescientist13 reviewed Dec 5, 2024

View reviewed changes

thescientist13 removed a link to an issue Dec 5, 2024

Issue Setting Element Properties #170

Closed

Added support for serializableShadowRoots and serializable

f88270a

Added test for create-document-fragment Other minor tweaks

briangrider closed this Jan 17, 2025

briangrider force-pushed the master branch from f88270a to 98a91a7 Compare January 17, 2025 17:29

Merge commit '98a91a7db8eeb1a4c067c07e8a48f25ad13970e0'

9ea019b

# Conflicts: # src/dom-shim.js # src/wcc.js

briangrider reopened this Jan 17, 2025

Added test case for shadowrootmode open vs. closed

f1d726c

thescientist13 reviewed Jan 17, 2025

View reviewed changes

thescientist13 linked an issue Jan 17, 2025 that may be closed by this pull request

Issue Setting Element Properties #170

Closed

thescientist13 changed the title ~~Rework of DOM shim (using parse5) to allow for element properties in the future~~ enhancement/Rework of DOM shim (using parse5) to allow for element properties in the future Jan 17, 2025

thescientist13 changed the title ~~enhancement/Rework of DOM shim (using parse5) to allow for element properties in the future~~ enhancement/rework of DOM shim (using parse5) to allow for element properties in the future Jan 17, 2025

thescientist13 previously requested changes Jan 17, 2025

View reviewed changes

thescientist13 mentioned this pull request Jan 18, 2025

bug/issue 1375 preserve HTML entities when prerendering markdown ProjectEvergreen/greenwood#1376

Merged

6 tasks

Made requested fixes and added additional tests

f49390b

thescientist13 reviewed Jan 19, 2025

View reviewed changes

Reverted light-dom test

7c8877d

thescientist13 approved these changes Jan 20, 2025

View reviewed changes

thescientist13 merged commit 36dd797 into ProjectEvergreen:master Jan 20, 2025

This was referenced Jan 20, 2025

bug/preserve light dom html entities in text nodes #182

Closed

DOM Shim is automatically inferring serializable option as true when calling attachShadow #179

Closed


		import { parseFragment, serialize } from 'parse5';

		// TODO Should go into utils file?

Conversation

briangrider commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue

Summary of Changes

Uh oh!

thescientist13 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briangrider commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thescientist13 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briangrider commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thescientist13 left a comment

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

thescientist13 left a comment

Choose a reason for hiding this comment

Uh oh!

thescientist13 Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thescientist13 left a comment

Choose a reason for hiding this comment

Uh oh!

thescientist13 left a comment

Choose a reason for hiding this comment

Uh oh!

briangrider commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

briangrider commented Nov 21, 2024 •

edited

Loading

thescientist13 left a comment •

edited

Loading

briangrider commented Nov 22, 2024 •

edited

Loading

thescientist13 left a comment •

edited

Loading

briangrider commented Jan 17, 2025 •

edited

Loading

thescientist13 Jan 19, 2025 •

edited

Loading