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.

Sign up for GitHub

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

feature/issue 177 Replaced deprecated method getInnerHTML with getHTML #171

Merged
merged 2 commits into from
Dec 20, 2024
Merged

feature/issue 177 Replaced deprecated method getInnerHTML with getHTML #171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e47238d
- replaced deprecated method getInnerHTML with getHTML and made this …
DannyMoerkerke Nov 5, 2024
3b334fb
updated example docs
thescientist13 Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/pages/examples.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export default async function (request, context) {
<html lang="en">
<body>
<wc-greeting>
${greeting.getInnerHTML({ includeShadowRoots: true })}
${greeting.getHTML({ serializableShadowRoots: true })}
<details slot="details">
<pre>
${JSON.stringify(context.geo)}
Expand Down
13 changes: 4 additions & 9 deletions src/dom-shim.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

// https://developer.mozilla.org/en-US/docs/Web/API/Node
// EventTarget <- Node
// TODO should be an interface?

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO should be an interface?'

Check warning on line 20 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO should be an interface?'
class Node extends EventTarget {
// eslint-disable-next-line
cloneNode(deep) {
Expand Down Expand Up @@ -45,10 +45,9 @@
return this.shadowRoot;
}

// https://github.com/mfreed7/declarative-shadow-dom/blob/master/README.md#serialization
// eslint-disable-next-line
getInnerHTML() {
return this.shadowRoot ? this.shadowRoot.innerHTML : this.innerHTML;
getHTML({ serializableShadowRoots = false }) {
return this.shadowRoot && serializableShadowRoots ?
Copy link
Member

@thescientist13 thescientist13 Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So did some initial testing in Greenwood and observed one small caveat here, with this line in particular

return this.shadowRoot && ...

In that technically you can attachShadow and return the value, for example

export default class HeaderComponent extends HTMLElement {
  connectedCallback() {
    this.root = this.attachShadow({ mode: 'open' });
    this.root.innerHTML = `<header>This is the header component.</header>`;
  }
}

customElements.define('app-header', HeaderComponent);

Would work, at least as demonstrated by MDN.

And so with this change we would assume users to always have to do either one of the following it seems, in being explicit about having this.shadowRoot present?

// option 1
this.attachShadow({ mode: 'open' });
this.shadowRoot.innerHTML = `<header>This is the header component.</header>`;

// option 2
this.shadowRoot = this.attachShadow({ mode: 'open' });
this.shadowRoot.innerHTML = `<header>This is the header component.</header>`;

I think it's not that big of a deal, and if so I can make sure we cover this as part of #181

@briangrider not sure if your PR would account for this, or should we just assume this.shadowRoot?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I think I'm not understanding the issue here. I believe everything you mentioned (including this.root = this.attachShadow({ mode: 'open' });) should work as expected both before the DOM shim refactor and after, right? If this.attachShadow is called, this.shadowRoot will always exist and both DOM shims return this.shadowRoot when attaching shadow. But I think it's possible I'm not understanding the problem.

Copy link
Member

@thescientist13 thescientist13 Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry yes, should have provided a bit more of the context but the test case that looked like it was failing was for this custom element definition
https://github.com/ProjectEvergreen/greenwood/blob/master/packages/cli/test/cases/build.config.prerender/src/components/header.js

In that I thought it was not emitting the <template> tag in the SSR output

<app-header>
  <header>This is the header component.</header>
</app-header>

But testing again after double checking my patching of this changeset and it does look the <template> tag is there

<app-header>
  <template shadowrootmode="open">
    <header>This is the header component.</header>
  </template>
</app-header>

and so the test case just needs to be updated to account for the (new) <template> which now appears and is the (new) expected behavior we would be seeing, yes? (the selector just needs to account for addition child element of the <template>

So apologies, may have been a false alarm. 🙃

Copy link
Contributor Author

@DannyMoerkerke DannyMoerkerke Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

According to the specs, attachShadow already returns this.shadowRoot: https://dom.spec.whatwg.org/#ref-for-dom-element-attachshadow①

`<template shadowrootmode="open">${this.shadowRoot.innerHTML}</template>` : this.innerHTML;
}

setAttribute(name, value) {
Expand Down Expand Up @@ -113,14 +112,10 @@
this.content = new DocumentFragment();
}

// TODO open vs closed shadow root

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO open vs closed shadow root'

Check warning on line 115 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO open vs closed shadow root'
set innerHTML(html) {
if (this.content) {
this.content.innerHTML = `
<template shadowrootmode="open">
${html}
</template>
`;
this.content.innerHTML = html;
}
}

Expand All @@ -132,13 +127,13 @@
// https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry
class CustomElementsRegistry {
constructor() {
// TODO this should probably be a set or otherwise follow the spec?

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably be a set or...'

Check warning on line 130 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably be a set or...'
// https://github.com/ProjectEvergreen/wcc/discussions/145
this.customElementsRegistry = new Map();
}

define(tagName, BaseClass) {
// TODO this should probably fail as per the spec...

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (20) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected 'todo' comment: 'TODO this should probably fail as per...'

Check warning on line 136 in src/dom-shim.js

View workflow job for this annotation

GitHub Actions / build (18) 

Unexpected ' TODO' comment: 'TODO this should probably fail as per...'
// e.g. if(this.customElementsRegistry.get(tagName))
// https://github.com/ProjectEvergreen/wcc/discussions/145
this.customElementsRegistry.set(tagName, BaseClass);
Expand Down
10 changes: 5 additions & 5 deletions src/wcc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ async function renderComponentRoots(tree, definitions) {
if (elementInstance) {
const hasShadow = elementInstance.shadowRoot;
const elementHtml = hasShadow
? elementInstance.getInnerHTML({ includeShadowRoots: true })
? elementInstance.getHTML({ serializableShadowRoots: true })
: elementInstance.innerHTML;
const elementTree = parseFragment(elementHtml);
const hasLight = elementTree.childNodes > 0;
Expand Down Expand Up @@ -222,14 +222,14 @@ async function initializeCustomElement(elementURL, tagName, node = {}, definitio

attrs.forEach((attr) => {
elementInstance.setAttribute(attr.name, attr.value);

if (attr.name === 'hydrate') {
definitions[tagName].hydrate = attr.value;
}
});

await elementInstance.connectedCallback();

return elementInstance;
}
}
Expand All @@ -244,7 +244,7 @@ async function renderToString(elementURL, wrappingEntryTag = true, props = {}) {
// in case the entry point isn't valid
if (elementInstance) {
const elementHtml = elementInstance.shadowRoot
? elementInstance.getInnerHTML({ includeShadowRoots: true })
? elementInstance.getHTML({ serializableShadowRoots: true })
: elementInstance.innerHTML;
const elementTree = getParse(elementHtml)(elementHtml);
const finalTree = await renderComponentRoots(elementTree, definitions);
Expand Down
4 changes: 1 addition & 3 deletions test/cases/attributes/src/components/counter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class Counter extends HTMLElement {
}

hydrate() {
this.count = parseInt(JSON.parse(this.shadowRoot.querySelector('script[type="application/json"').text).count, 10);
this.count = parseInt(JSON.parse(this.shadowRoot.querySelector('script[type="application/json"]').text).count, 10);

const buttonDec = this.shadowRoot.querySelector('button#dec');
const buttonInc = this.shadowRoot.querySelector('button#inc');
Expand All @@ -54,13 +54,11 @@ class Counter extends HTMLElement {

render() {
return `
<template shadowrootmode="open">
<div>
<button id="inc">Increment</button>
<span>Current Count: <span id="count">${this.#count}</span></span>
<button id="dec">Decrement</button>
</div>
</template>
`;
}
}
Expand Down
20 changes: 9 additions & 11 deletions test/cases/get-data/src/components/counter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Counter extends HTMLElement {

hydrate() {
console.debug('COUNTER => hydrate');
this.count = parseInt(JSON.parse(this.shadowRoot.querySelector('script[type="application/json"').text).count, 10);
this.count = parseInt(JSON.parse(this.shadowRoot.querySelector('script[type="application/json"]').text).count, 10);

const buttonDec = this.shadowRoot.querySelector('button#dec');
const buttonInc = this.shadowRoot.querySelector('button#inc');
Expand All @@ -52,17 +52,15 @@ class Counter extends HTMLElement {

render() {
return `
<template shadowrootmode="open">
<script type="application/json">
${JSON.stringify({ count: this.count })}
</script>
<script type="application/json">
${JSON.stringify({ count: this.count })}
</script>

<div>
<button id="inc">Increment</button>
<span>Current Count: <span id="count">${this.count}</span></span>
<button id="dec">Decrement</button>
</div>
</template>
<div>
<button id="inc">Increment</button>
<span>Current Count: <span id="count">${this.count}</span></span>
<button id="dec">Decrement</button>
</div>
`;
}
}
Expand Down
38 changes: 18 additions & 20 deletions test/cases/nested-elements/src/components/header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,27 @@ class Header extends HTMLElement {

render() {
return `
<template shadowrootmode="open">
<header class="header">
<div class="head-wrap">
<div class="brand">
<a href="/">
<img src="/www/assets/greenwood-logo.jpg" alt="Greenwood logo"/>
<h4>My Personal Blog</h4>
</a>
</div>
<header class="header">
<div class="head-wrap">
<div class="brand">
<a href="/">
<img src="/www/assets/greenwood-logo.jpg" alt="Greenwood logo"/>
<h4>My Personal Blog</h4>
</a>
</div>

<wcc-navigation></wcc-navigation>
<wcc-navigation></wcc-navigation>

<div class="social">
<a href="https://github.com/ProjectEvergreen/greenwood">
<img
src="https://img.shields.io/github/stars/ProjectEvergreen/greenwood.svg?style=social&logo=github&label=github"
alt="Greenwood GitHub badge"
class="github-badge"/>
</a>
</div>
<div class="social">
<a href="https://github.com/ProjectEvergreen/greenwood">
<img
src="https://img.shields.io/github/stars/ProjectEvergreen/greenwood.svg?style=social&logo=github&label=github"
alt="Greenwood GitHub badge"
class="github-badge"/>
</a>
</div>
</header>
</template>
</div>
</header>
`;
}
}
Expand Down
Loading