Fix linking of anchors in JSDoc content #14558
Conversation
| // If storybook is hosted at a non-root path (e.g. `/storybook/`), | ||
| // the base url needs to be prefixed to storybook paths. | ||
| let storybookBaseUrl = | ||
| typeof window !== 'undefined' ? window.parent.document.location.pathname : '/'; |
There was a problem hiding this comment.
i believe window.parent causes a CORS error in composed storybooks cc @ndelangen
There was a problem hiding this comment.
The preview cannot 'hook' directly into the manager and vice versa.
There was a problem hiding this comment.
Thank you for the comments! Could you elaborate a bit what exactly the problem is and how we can solve it? I'm not really familiar with Storybook internals.
There was a problem hiding this comment.
window.parent is an illegal operation when the frame is on another origin @amannn
There was a problem hiding this comment.
Good point, thanks!
I just tried it, the precise error is:
Uncaught DOMException: Blocked a frame with origin "http://localhost:8081" from accessing a cross-origin frame.
What would be a good strategy to handle this? Should we use try/catch to try to use the parent location and otherwise fall back to the default linking, which is based on the inner iframe?
I'm not sure how common it is that the inner iframe is on a separate domain. At least for the Storybook installation I'm maintaining it is the same one.
There was a problem hiding this comment.
@ndelangen Did you have a chance to test this as described above?
There was a problem hiding this comment.
Is there anything I can do to help with this?
There was a problem hiding this comment.
I haven't had time to dive into this.. I understand the code changes in your PR fixes the issue, but it creates new ones, where CORS error occur. We need another way of fixing the issue. accessing window.parent, is not an option, unfortunately.
There was a problem hiding this comment.
Thanks for your reply! I've just rebased the branch and wrapped the logic with a try/catch as discussed above. As you mention, it doesn't fix the situation for composed storybooks though – only for non-composed ones.
Would it make sense to have this as a first fix and work on a more involved situation for composed storybooks separately? I currently don't know enough about the architecture of Storybook to propose a more general fix …
There was a problem hiding this comment.
@ndelangen I've rebased this PR based on the current next branch. Is there anything I could do to help get this merged? It seems like some unrelated CI checks are failing.
| // If storybook is hosted at a non-root path (e.g. `/storybook/`), | ||
| // the base url needs to be prefixed to storybook paths. | ||
| let storybookBaseUrl = | ||
| typeof window !== 'undefined' ? window.parent.document.location.pathname : '/'; |
There was a problem hiding this comment.
I haven't had time to dive into this.. I understand the code changes in your PR fixes the issue, but it creates new ones, where CORS error occur. We need another way of fixing the issue. accessing window.parent, is not an option, unfortunately.
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 8b30e9d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks! |
amannn
force-pushed
the
fix/docs-anchors
branch
from
9a45508 to
696998b
Compare
# Conflicts: # lib/components/src/typography/DocumentFormatting.tsx
| if (typeof window !== 'undefined') { | ||
| try { | ||
| storybookBaseUrl = window.parent.document.location.pathname; | ||
| } catch (error) { | ||
| // For composed storybooks this doesn't work due to a CORS error | ||
| } |
|
@amannn We really appreciate the time, effort and energy you put into this PR. I understand it's never fun to see pull-requests closed. I think in this case it's because we're going to do a big improvement for docs in 7.0 plus the fact we don't want to make much more changes to 6.x unless it's for security or critical bugs. @tmeasday or @shilman let's plan to chat about this at the next roadmap meeting? |
Issue: #11404
What I did
Somehow the behaviour I added in #11403 got removed at some point. Additionally linking to other stories was broken for me for JSDoc content in general, since the anchor needs to start with a
/. Otherwise the URL of the top iframe is used as the base and you end up at a wrong location.How to test
You can test this in the new story I added.