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

fix(v2): linking to asset or external html page -> don't use history.push() #3347

Merged
merged 6 commits into from
Aug 28, 2020

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Aug 27, 2020

Motivation

We should be able to use the existing webpack file-loaders when linking to assets in the static folder, and we should be able to link using the @site webpack alias.

We should only navigate to a markdown link using history.push() if that link is a known SPA route. We should not "navigate" to a pdf file (#3337), or an external HTML file that is not part of the SPA (#3309)

In the future, maybe the Link component should be able to know if a path like /javadoc is internal or external by matching it to known routes, but I don't think it's ideal either because it has a runtime cost, and it would also opt-out of broken link detection, as unknown routes would be considered as external and bypass this check.

The pathname:// protocol is an escape hatch that permits to tell Link that a link is expected to be considered as external. It is more explicit.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Unit tests

Dogfooding on a dedicated page:

@slorber slorber requested a review from yangshun as a code owner August 27, 2020 16:46
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Aug 27, 2020

Deploy preview for docusaurus-2 ready!

Built with commit c6facc3

https://deploy-preview-3347--docusaurus-2.netlify.app

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 27, 2020
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Aug 27, 2020
@slorber slorber merged commit c7fc781 into master Aug 28, 2020
@lex111 lex111 deleted the slorber/markdown-tests-page branch October 7, 2020 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants