Element should not auto-register itself unless a browser context is found

[iansan5653]

Per discussion in https://github.com/primer/react/pull/2339, the fact that this element automatically registers itself at the top level makes it incompatible with server-side rendering, where JavaScript might be run outside of a browser context.

From an SSR perspective, adding a simple typeof window !== 'undefined' guard should be sufficient. However, that won't solve the fact that the markdown-toolbar-element currently executes with side-effects, and thus cannot be tree-shaken. This means bundles in dotcom that do not need toolbar element will carry it as extra weight that is not needed. We would need to add a register() function to allow tree shaking, which would be a breaking change on the web component package.

[koddsson]

Could you talk more about server-side rendering? Since this element doesn't actually render in a JS context I'm surprised at how it wouldn't work. A reduced test case for server side rendering would be really helpful since I'm not sure what server-side rendering means in this context.

[koddsson]

I've tried reproducing this in both next.js and react-dom/server and couldn't get this to blow up so a reduced test case would be much appriciated.

[iansan5653]

Sure, you can try running npm run build:docs in this branch of primer/react.

The problem is not the element itself rendering, but the fact that it immediately calls unavailable DOM APIs upon import.

[koddsson]

I don't get a error related to a window in my reduced Gatsby case. What is the error you are getting? I can't get primer/react up and running.

I get a error related to HTMLElement which Gatsby points me to https://www.gatsbyjs.com/docs/debugging-html-builds/ which points to multiple solutuions to this issue. I'll see about getting a PR to primer/react with one of these solutions.

[joshblack]

Could you talk more about server-side rendering? Since this element doesn't actually render in a JS context I'm surprised at how it wouldn't work

I believe the challenge is that the package entrypoint makes references to browser globals that are not available in a Node.js context. When Next.js tries to SSR a page, it encounters a reference to HTMLElement, for example, which is undefined. The package itself is evaluated due to the consumer importing the package:

import '@github/markdown-toolbar-element';

export default function IndexPage() {
  // ...
}

For a quick repro on the Next.js side, here's a zip:

nextjs.zip

Steps to reproduce

• Unzip the file
• Run npm i
• Run npm run develop

The following error should show up when visiting http://localhost:3000:

This comes from importing @github/markdown-toolbar-element in pages/index.js. The specific line the stack trace points to is line 42 which includes HTMLElement: class MarkdownButtonElement extends HTMLElement {

[koddsson]

This renders for me with the markdown toolbar custom elements registered and that demo running withing out crashing.

import React, { useEffect } from "react";

export default function IndexPage() {
  useEffect(() => {
    (async function() {
      await import('@github/markdown-toolbar-element');
    })()
  })

  return 'index';
}

[joshblack]

@koddsson definitely agreed, shifting it to a dynamic import in useEffect() will run it in a client context 👍 On my end, I wanted to share a use-case/example for the SSR case.

[iansan5653]

await import() causes a segfault, at least in Jest tests. See the CI runs for this PR https://github.com/primer/react/pull/2334 where I've done exactly this