-
Notifications
You must be signed in to change notification settings - Fork 57
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
Comments
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. |
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. |
Sure, you can try running The problem is not the element itself rendering, but the fact that it immediately calls unavailable DOM APIs upon import. |
I don't get a error related to a I get a error related to |
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 import '@github/markdown-toolbar-element';
export default function IndexPage() {
// ...
} For a quick repro on the Next.js side, here's a zip: Steps to reproduce
The following error should show up when visiting This comes from importing |
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';
} |
@koddsson definitely agreed, shifting it to a dynamic import in |
|
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.
The text was updated successfully, but these errors were encountered: