feature/issue 177 Replaced deprecated method getInnerHTML with getHTML

[DannyMoerkerke]

Summary of Changes

• replaced deprecated method getInnerHTML with getHTML and made this return the HTML wrapped in a DSD template when the serializableShadowRoots option is set to true, which is according to the specs
• removed adding <template shadowrootmode="open"> when innerHTML of <template> is set since this will now correctly be added when the getHTML method of the element is called with the option { serializableShadowRoots: true }
• updated tests
• added missing closing square bracket in selector: this.shadowRoot.querySelector('script[type="application/json"')

This change makes sure that a web component that has its Shadow DOM set through innerHTML instead of a template like this:

const shadowRoot = this.attachShadow({mode: 'open'});
shadowRoot.innerHTML = `...`;

can also be server-side rendered.

[thescientist13]

Thanks for this @DannyMoerkerke 👋

I'll have to do a little research into getHTML and certainly happy to favor it over using getInnerHTML which itself seems fine on the surface, but not sure I quite understand the full implications of the behavior change to specifically auto render out the <template> tag.

For a change of this nature though it would good for us to chat through it in an issue first. (I find PR review format tough for having these more in depth conversations, and then we can leave the PR for only the final code implementation and not all the backstory too). I have a few questions / thoughts off the top of my head, in particular as to which parts of this change are and aren't part of the spec and if there any implications on currently assumed behavior of WCC, and if any of this would be a breaking change.

Mostly just want to make sure I am following along correctly because I think this is certainly an interesting proposal, but also want to provide any meaningful feedback in the context of my own projects using WCC as well.

Excited to explore this further and thanks for checking out WCC! 🙏

[DannyMoerkerke]

Sure Owen, happy to chat further! Let’s schedule something at your convenience

[thescientist13]

So coming out of chats in #177, here are the items we would need to get this one in

1. Need to update the documentation here (or anywhere else getInnerHTML is used to reflect the API change - https://merry-caramel-524e61.netlify.app/examples/#serverless-and-edge-functions
2. also need to validate all the sandbox examples are still working (just checked these locally and they look good to me)
3. Smoke test this change against Greenwood (will spin up a branch now and report back shortly)

[thescientist13]

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?

[briangrider]

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.

[thescientist13]

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

[DannyMoerkerke]

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

[thescientist13]

Here's the draft PR for Greenwood for anyone who is curious, so I think we're all good on the downstream side of things after I tweaked the test case to account for the new <template> tag behavior.
ProjectEvergreen/greenwood#1340

So I think once we get docs item updated, we should be good to merge this. 💯

[thescientist13]

Hey @DannyMoerkerke , let me know if you're able to make the docs update, otherwise happy to take it on in a couple days.

Just want to help land this so the next couple PRs in the queue can start building off this 👍

[DannyMoerkerke]

@thescientist13 sure, we're talking about https://merry-caramel-524e61.netlify.app/examples/#serverless-and-edge-functions right?

[thescientist13]

@DannyMoerkerke
Yup! I think that's the only one I found but a simple find / replace for getInnerHTML -> getHTML should catch any I may have missed. 🔍

[DannyMoerkerke]

I’ll go over all the docs.

[thescientist13]

OK! I went ahead and made the docs updated and rebased this branch against upstream master and everything looks good to go.

Thanks for the contribution @DannyMoerkerke and tag @briangrider , you're it! 😄