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

<Footnotes /> Render bugs with GatsbyJS #48

Open
8bitmatt opened this issue Aug 6, 2021 · 7 comments
Open

<Footnotes /> Render bugs with GatsbyJS #48

8bitmatt opened this issue Aug 6, 2021 · 7 comments

Comments

@8bitmatt
Copy link

8bitmatt commented Aug 6, 2021

First of all, thank you for making this! ❤️

The problems

  1. The last FootnoteRef that uses jsx in the description (instead of a string) gets duplicated when clicking on "data-a11y-footnotes-ref" or "data-a11y-footnotes-back-link" (basically when Footnotes is triggered to re-render)

  2. If you customize the Footnotes Wrapper, the component renders multiple times. Which either completely breaks the :target css selector or makes it behave inconsistently (tested in Chrome, Firefox, and Safari)

I tried looking through the code in this repo, and I imagine this has something to do with the SSR code (registering, cloning, etc.) but I don't know for sure. This is a little over my head right now, so I thought I'd reach out for help.

gatsby-render-bugs

Steps to reproduce

Using Gatsby cli start a new project gatsby new

Install this package (react-a11y-footnotes), update the src/pages/index.js with basic implementation

<FootnotesProvider>
  <p>Just <FootnoteRef description="My footnote 1">testing footnotes</FootnoteRef></p>
  <p>...and <FootnoteRef description={<>My footnote 2</>}>testing more footnotes</FootnoteRef></p>
  {/* <p>some more <FootnoteRef description={<span>My footnote 3</span>}>footnotes</FootnoteRef></p> */}
  <Footnotes Wrapper={props => <div {...props} className="footnotes">{props.children} {console.warn('render footnotes')}</div>} />
</FootnotesProvider>

  • Remove the Wrapper prop and see the :target css selector start working again.
  • Uncomment the 3rd footnote and see that the duplication problem now happens on that instead of the 2nd footnote.
@KittyGiraudel
Copy link
Owner

Hello Matt!

Thank you for reaching out and for the detailed bug report. I’ve been having a look at what’s going on, and I can confirm it has nothing to do with Gatsby itself. It would happen on any system using client-side rendering. It can easily be reproduced on CodeSandbox as well for instance.

What I found out is that the problem only happens with footnotes using JSX for their description prop. String footnotes are working as they should. The reason why is that JSX trees don’t pass the memoization (from useMemo) as they’re essentially always different. For that reason the footnotes using JSX get re-rendered when the parent component updates.

An actionable fix (until I figure out how to solve it on the lib side) is to store the JSX tree in a ref so it’s stable across renders.

const MyComponent = () => {
  const footnoteDescription = React.useRef(<>My footnote 2</>)

  return (
    <FootnotesProvider>
      <FootnoteRef description={footnoteDescription.current}>testing more footnotes</FootnoteRef>
      <Footnotes />
    </FootnotesProvider>
  )
}

@KittyGiraudel
Copy link
Owner

Okay, nevermind my previous comment, it was a load of shit. Turns out I can’t type and there was a typo in the code. Shame on me. 😅 This has been fixed in 0.4.3 and I think (I hope) things will be smoother for you.

https://github.com/KittyGiraudel/react-a11y-footnotes/releases/tag/0.4.3

Could you update and let me know if that solves your problem? :)

@8bitmatt
Copy link
Author

8bitmatt commented Aug 6, 2021

Thank you! This fixed the duplication problem 🥳🙌

The 2nd issue still exists. Customizing the Wrapper breaks the css :target
In my simple test (with a custom Wrapper).
<Footnotes Wrapper={props => <div {...props} className="footnotes">{props.children}</div>} />

I can see the DOM re-renders every time I click a different footnotes link (I see Chrome dev tools highlighting the changed DOM nodes in the Elements tab). Clicking the same link twice in a row will not re-render and the target styles show.

When the Wrapper is not customized.
<Footnotes />
The DOM doesn't re-render, so target styles always work as expected

@8bitmatt
Copy link
Author

8bitmatt commented Aug 7, 2021

I also found that customizing only the ListItem causes the same problem.
<Footnotes ListItem={props => <li {...props} />} />

@KittyGiraudel
Copy link
Owner

@8bitmatt Could you try memoizing your components please? Like so:

const FootnotesWrapper = React.memo(props => <div {...props} className="footnotes">{props.children}</div>)
const FootnotesListItem = React.memo(props => <li {...props} />)

<Footnotes Wrapper={FootnotesWrapper} ListItem={FootnotesListItem} />

@8bitmatt
Copy link
Author

8bitmatt commented Aug 9, 2021

React.memo does not seem to help. (If the assignment happens inside the main exported component)

const IndexPage = () => {
    const FootnotesWrapper = React.memo(props => <div {...props} className="footnotes">{props.children}</div>)
    const FootnotesListItem = React.memo(props => <li {...props} />)

return (
    ...
)
};

export default IndexPage

If I move the assignment outside of the component it does help... But, I also tried removing React.memo in that situation and it works too. 🤷

// Seems to help
const FootnotesWrapper = React.memo(props => <div {...props} className="footnotes">{props.children}</div>)
const FootnotesListItem = React.memo(props => <li {...props} />)

const IndexPage = () => {
...
};
// But this works too
const FootnotesWrapper = props => <div {...props} className="footnotes">{props.children}</div>
    
const IndexPage = () => {
...
};

Anyway, I can get by with this! Thank you for helping!

@KittyGiraudel
Copy link
Owner

Hello! 👋 Sorry I’m only now coming back to this.

I think what you explained makes sense: if you declare components within the main component, you end up passing different components to the footnotes for every render, which can cause issues. By declaring them outside of the render lifecycle, they‘re always the same. :)

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

2 participants