[docs] Fix heading fragment id mismatch #20520
Conversation
Details of bundle changes.Comparing: dc0578c...f1b7414 Details of page changes
|
There was a problem hiding this comment.
An example of a K.O case after the change:
- Navigate to https://deploy-preview-20520--material-ui.netlify.com/guides/interoperability/
- Click on
Controlling priority, the one highlighted in the below screenshot. The link doesn't work. The id of the table of contents and the DOM doesn't match.
I could track down the origin of the singleton to #14703.
eps1lon
force-pushed
the
docs/strict-mode-switch
branch
from
851c4c9 to
f1b7414
Compare
|
I'm not aware of an easy solution, I'm sorry for leaving this 🥚here. Ideally, we wouldn't have introduced this global in the first place. |
|
I was expecting some weirdness. I tested it on https://deploy-preview-20520--material-ui.netlify.com/styles/api/#arguments and that one seems fine even though it should clash just as well. My ideal solution would be an id that is generated from the heading path. That should be unique without any special counter logic. Or my actual ideal solution would be moving markdown parsing to build time. Then we could do two passes. I'm leaving this open for a bit in case I stumble over an easy solution. It's not that important. Just helps debugging concurrent mode issues by reducing noise. |
What mode should we aim for using by default in the documentation (long term)? |
The stable one which is The others require |
|
Let's close this for now. I don't have an easy solution to making this concurrent safe. Especially since we already render the markdown in chunks when it contains demos. With mdx we can build the toc at build time (assuming all headings are written in markdown). |
|
Won't we be able to generate the HTML from the markdown at build time too with marked? I would imagine we could cache the output and store it in getInitialProps/getStaticProps instead of importing the markdown source. |
|
Yeah that would work too. But there are other problems as well so I want to get an overview of what is problematic first before spending too much time on a single issue. Working on the StrictMode compatible transitions was driven by too much wishful thinking 😉 |
Actual fix: c0f6c7d
The rest is a refactoring in next.config.js to use their API to switch between various render modes of React.
global.__MARKED_UNIQUE__was problematic since it resulted intextToHash(..., global.__MARKED_UNIQUE__)to become impure and therefore unsuitable for rendering in react. In addition to that we also reset it during a different render making it even more problematic.Removing it seems fine. We still avoid heading-id collisions since we reset the id cache on every render. This is the only concurrent-safe usage of
textToHash: If it's called during renderuniquemust be reset before that render call. It can only be "global" in the scope of the render call of a particular component. It cannot be re-used between two different components.