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

feat(v2): onBrokenMarkdownLinks config #3658

Merged
merged 6 commits into from
Oct 31, 2020
Merged

Conversation

AmyrAhmady
Copy link
Contributor

Motivation

Fixing issue #3652

Little additional info:

We have a website which uses Docusaurus v2, and recently, we are letting people to contribute to the project and translate existing pages to their own language in separate folders; This means relative markdown links are still there in newly translated pages while those pages might not exist in the language's folder (as I mentioned, those links are relatively written there)

An example of this behaviour would be this: Click Here, and first item of that list does not exist. This was just an example that leads to printing only one error to stdout, for the real issue please refer to #3652

And at the end, there was a comment there saying this: https://github.com/facebook/docusaurus/blob/master/packages/docusaurus-plugin-content-docs/src/index.ts#L314

Have you read the Contributing Guidelines on pull requests?

I'd be lying if I say I've read it fully and carefully, but I tried to cherry pick needed parts but scrolling through, sorry :)

Test Plan

Not sure how exactly we are supposed to test that, but I guess running an instance of Docusaurus with "onBrokenLinks": "ignore" in site configurations and a dummy doc page with a broken relative link would be fine.

Here's a screenshot (I've heard rumors it has bonus points!)
Docusaurus SS

Related PRs

None I guess?

@facebook-github-bot
Copy link
Contributor

Hi @AmyrAhmady!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 30, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 16ad52a

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

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi and thanks for completing my todo :)
That looks pretty good overall.

There's for me one major problem: if you have 2 MD broken links and the throw config (recommended, default config), you might fail on first md broken link found.
In such case only one broken link would be reported: the first one.

We'd rather find a way to aggregate all the md broken links here instead of failing fast asap?
Maybe storing a mutable broken links list in the plugin, and reporting all the broken links once all the docs have been read?

If that's not possible, I'd rather have a 2nd option like siteConfig.onMarkdownBrokenLink or something, that would rather default to a warning, so that user might see all the md broken links in a single build attempt.

The important thing to me is that the user does not have to fail a build, fix 1 link, fail another build, fix another link etc... he should see all the broken links by default.

Does it make sense?

packages/docusaurus-plugin-content-docs/src/index.ts Outdated Show resolved Hide resolved
works like onBrokenLinks, but only for markdown links
@AmyrAhmady
Copy link
Contributor Author

AmyrAhmady commented Oct 31, 2020

I have just pushed two commits, containing changes and features you've asked above :)
What I did making broken markdown links independently configurable so we don't need to mix them up with actual broken links;
That being said, we now have a totally new option in site configs, called onBrokenMarkdownLinks and works exactly like onBrokenLinks, and its default value is set to warn

Let me know if there's anything needed

EDIT: Don't get me wrong, I'm doing force pushes to avoid having extra random commits for small fixes by doing amend, just wanted to have a clean contribution

@slorber
Copy link
Collaborator

slorber commented Oct 31, 2020

thanks, that looks great :)

No need to doc for alpha 66, as this will only be released in 67.

Also we squash all PRs so your git commit history does not matter much

@AmyrAhmady
Copy link
Contributor Author

AmyrAhmady commented Oct 31, 2020

Nice! thanks, also I know this isn't the right place to ask, but is there any planned release date for alpha 67?

@slorber
Copy link
Collaborator

slorber commented Oct 31, 2020

we plan to release during the next week

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Oct 31, 2020
@slorber slorber changed the title feat(v2): Add ability of changing what happens when markdown broken links are found feat(v2): onBrokenMarkdownLinks config Oct 31, 2020
@slorber slorber merged commit 8f2d898 into facebook:master Oct 31, 2020
@lex111 lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020
@zepatrik
Copy link

zepatrik commented Nov 18, 2020

Released with https://github.com/facebook/docusaurus/releases/tag/v2.0.0-alpha.68 🎉

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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants