staylor / react-helmet-async

Thread-safe Helmet for React 16+ and friends
Apache License 2.0
2.11k stars 154 forks source link

HelmetDispatcher leaks when used in combination with Suspense #226

Open satoren opened 5 months ago

satoren commented 5 months ago

If Suspense occurs after render, componentWillUnmount is not called. Therefore, instances added below will not be removed and will leak. https://github.com/staylor/react-helmet-async/blob/5f4da3896c8b70bedcd3c05d406ff5241601933c/src/Dispatcher.tsx#L71

Also, it will not be updated properly.

https://react.dev/reference/react/Component#componentdidmount

Reference Articles (This is not my post.)

https://zenn.dev/mtblue81/articles/f88821a3c3392e https://mtblue81.github.io/react-helmet-sample/

Maximaximum commented 4 days ago

Looks like I'm having this exact issue, which is quite annoying.

I'm using Suspense in my app, and sometimes the document title just doesn't get updated after a navigation, or it does get updated, but with a wrong title.

@staylor Could you please look into this issue? Currently it prevents me from even starting using react-helmet-async in my projects. 😢

satoren commented 4 days ago

I stopped using react-helmet-async and instead changed to the following simple implementation. In our use case, this was sufficient.

const DocumentTitle = ({title}: {title: string}) => {
  useEffect(()=>{
    document.title = title
  }, [title])
  return null
}
Maximaximum commented 2 days ago

I stopped using react-helmet-async and instead changed to the following simple implementation. In our use case, this was sufficient.

const DocumentTitle = ({title}: {title: string}) => {
  useEffect(()=>{
    document.title = title
  }, [title])
  return null
}

Thanks @satoren. Your solution does work for most basic scenarios, but it does not work for scenarios when a page does not have its own title. In such cases it's expected that the document should have the "default" title, set by a parent layout component up in the tree. However, using this simple solution you get the title defined by the latest activated page, regardless of the component hierarchy 😢

satoren commented 2 days ago

You can wait until react 19.

Or it could incorporate a few improvements, such as

const DocumentTitle = ({title}: {title: string}) => {
  useEffect(()=>{
    const prev = document.title
    document.title = title
    return ()=>{
       document.title = prev
    }
  }, [title])
  return null
}
// Or if you want to deal with more complex scenarios
const DocumentTitle = ({ title }: { title: string }) => {
  const [titleElement] = React.useState<HTMLTitleElement>(() => document.createElement('title'))
  useEffect(() => {
   // The title is inserted into the head in reverse order of rendering.
    document.head.prepend(titleElement);
    return () => {
      titleElement.remove()
    }
  }, [titleElement])

  useEffect(() => {
    titleElement.innerText = title
  }, [title, titleElement])
}