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

No support for imports from isomorphic scripts #230

Open
jonathandewitt-dev opened this issue Mar 16, 2022 · 3 comments
Open

No support for imports from isomorphic scripts #230

jonathandewitt-dev opened this issue Mar 16, 2022 · 3 comments

Comments

@jonathandewitt-dev
Copy link

jonathandewitt-dev commented Mar 16, 2022

I'm using this polyfill in basically every project now, you guys are life savers, so thanks for that!

I tend to use a lot of universal JavaScript frameworks, especially Next.js. One of the benefits of doing so is server-side rendering in Node in combination with client-side routing - in other words, isomorphic templating. However, there's a lot of things to be careful about when you run the same code on client and server, like when trying to access window. As you're probably aware, Node is very particular about references to window.

Hence, the references to window within this package's code cause our builds to fail. Without ever making a reference to dialogPolyfill, this static import causes an error by itself:

import dialogPolyfill from 'dialog-polyfill'

My only work-around so far has been a dynamic import, only after I'm sure the script is in a client-side context.

if (typeof window !== 'undefined') {
  const dialogPolyfill = await import('dialog-polyfill')
}

... but that isn't ideal for me, especially considering how difficult it can be to diagnose this problem.

I would much rather keep a consistent import style, then handle calling it on the client side myself. For example, in a typical Next component:

import { useRef, useEffect } from 'react'
import dialogPolyfill from 'dialog-polyfill'

const ReactDialog = () => {
  const dialogRef = useRef(null)

  useEffect(() => {
    dialogPolyfill.registerDialog(dialogRef.current)
  }, [])

  return <dialog ref={dialogRef}>Hello World</dialog>
}

Since there is only a benefit client-side, I might expect a clear error to be thrown if I inadvertently use the polyfill on the server side, but that level of error handling can help the consumer of the package to easily troubleshoot and solve the problem. Otherwise, the entire build fails with a less descriptive error complaining that window is undefined.

@jonathandewitt-dev
Copy link
Author

To address this issue, I've created a pull request myself, for your review: #231

@targos
Copy link

targos commented Jun 27, 2022

FWIW I published dialog-polyfill-universal for SSR compatibility.

@jamieomaguire
Copy link

Just wanted to add that I have experienced this exact issue. If you would consider reviewing the PR raised by @jonathandewitt-dev that would be incredible please ❤️

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