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

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 feature/issue 177 Replaced deprecated method getInnerHTML with getHTML #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 - support configurable shadowrootmode attribute for <template> tags #65
Looks like this PR would also help resolve or invalidate verify / ensure proper serialization of shadow roots excluding closed shadow roots from getInnerHTML #16 ?
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)

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

test/cases/shadowrootmode/shadowrootmode.spec.js

+
+  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

test/cases/shadowrootmode/shadowrootmode.spec.js

+ * 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

test/cases/create-document-fragment/create-document-fragment.spec.js

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

src/dom-shim.js

@@ -133,6 +252,14 @@ class CustomElementsRegistry {
  }

  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

src/dom-shim.js

+
+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

src/dom-shim.js

+}
+
+// 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

src/dom-shim.js

+}
+
+// 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 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

Open

Issue Setting Element Properties #170

Open

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

Open

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

Open

thescientist13 reviewed Dec 5, 2024

View reviewed changes

thescientist13 removed a link to an issue Dec 5, 2024

Issue Setting Element Properties #170

Open

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

Open

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

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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 left a comment

thescientist13 Jan 17, 2025

thescientist13 Jan 17, 2025

thescientist13 Jan 17, 2025

thescientist13 Jan 17, 2025

thescientist13 Jan 17, 2025

thescientist13 Jan 17, 2025

thescientist13 Jan 17, 2025

thescientist13 left a comment


		import { parseFragment, serialize } from 'parse5';

		// TODO Should go into utils file?

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

Are you sure you want to change the base?

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

Conversation

briangrider commented Nov 21, 2024 • edited Loading

Related Issue

Summary of Changes

thescientist13 left a comment • edited Loading

Choose a reason for hiding this comment

briangrider commented Nov 22, 2024 • edited Loading

thescientist13 left a comment • edited Loading

Choose a reason for hiding this comment

briangrider commented Jan 17, 2025 • edited Loading

thescientist13 left a comment

Choose a reason for hiding this comment

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

thescientist13 Jan 17, 2025

Choose a reason for hiding this comment

thescientist13 left a comment

Choose a reason for hiding this comment

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