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.

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

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

Open
iansan5653 opened this issue Sep 23, 2022 · 8 comments

Comments

@iansan5653
Copy link
Member

Per discussion in primer/react#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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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

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:

image

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
Copy link
Contributor

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

@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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants