onBrokenMarkdownImagePaths to catch bad paths without failing build

sserrata commented 3 years ago

🐛 Bug Report

The latest version of D2 fails to build if markdown links or images contain an empty URL value. A build may also fail if a relative URL value path cannot be resolved.

Have you read the Contributing Guidelines on issues?

Yes.

To Reproduce

(Write your steps here:)

Add a markdown link or image to a doc with an empty URL value.
Attempt to perform a dev server start or production build.

Expected behavior

Inherit the onBrokenMarkdownLinks setting. For example, if set to warn then error should be displayed as a warning and dev server or build should continue.

(Write what you thought would happen.)

Actual Behavior

Seeing the following errors:

Error: Markdown link url is mandatory.
Error: Markdown Image url is mandatory.

(Write what happened. Add screenshots, if applicable.)

Your Environment

Docusaurus version used: alpha 70
Environment name and version (e.g. Chrome 78.0.3904.108, Node.js 10.17.0): Chrome 87.0.4280.88, Node.js 14.x
Operating system and version (desktop or mobile): MacOSX 10.15.7

Reproducible Demo

https://github.com/demisto/content-docs/pull/429

RDIL commented 3 years ago

I think this is remark or something like it. Either way, the setting is supposed to check for broken links, not no links at all. Not sure what should be done here.

slorber commented 3 years ago

Hi,

You upgrade from alpha 58 to 70, there were quite a few changes to make Docusaurus more "fail fast" recently, preventing you from deploying broken links, image paths, and using bad config.

It is better to discuss the issues you have one by one.

onBrokenMarkdownLinks will detect bad links in markdown documents, while onBrokenLinks is more "global" and will look at any link of your whole site. A broken markdown link generally leads to a regular broken link, so you'd rather set both options to warn if you prefer deploying broken links rather than failing fast.

Regarding empty markdown links and image URLs error, the behavior is the expected one to me. What are your motivations for using empty URLs anyway? Shouldn't this be fixed in your docs instead of being allowed by Docusaurus?

The "URL is mandatory" error is not related to the onBrokenMarkdownLinks setting. onBrokenMarkdownLinks is to catch errors where you have an actual link like [myLink](./docThatDoesNotExist.md)

Does it make sense?

I'm not against allowing empty URLs again if you present me a decent use-case for allowing empty urls

sserrata commented 3 years ago

Hi @slorber, I think the root of the issue comes down to how "broken" markdown links and image URLs/paths are defined and eventually handled by D2.

Given the following examples:

[myLink](./docThatDoesNotExist.md)

and

[myLink]()

Technically, both are broken markdown links but one of them (the empty URL/path) is a more egregious misuse/abuse of the markdown syntax. I can understand the reason for checking if the URL/path is present before attempting to resolve it, but I'm not 100% sure empty URLs/paths need to be handled in a completely separate way. When I first saw the error, I immediately set onBrokenMarkdownLinks to "warn" thinking that it would allow the build to continue (I could be in the minority here).

I guess what I'm proposing is that empty URLs/paths be handled within the same scope as the onBrokenMarkdownLinks logic/checks.

I agree with you in that there isn't a legitimate use case for allowing empty URLs/paths but I could make the same argument for having broken markdown links in general. I think the more important thing to accept is that broken markdown links happen, and it's typically not intentional.

For example (In the case of our site), the content itself is being pulled in from an external repo that accepts open source contributions. Basically, some CI/CD automation pulls and generates docs (from this external repo) as part of the D2 build. I believe that these contributors might be following a predefined template that ends with asking for an image, which, in some cases, could be intentionally left blank as a "placeholder" of sorts. I'm not saying this is a legitimate use case - just highlighting how mistakes can and will happen.

slorber commented 3 years ago

onBrokenMarkdownLinks is supposed to be used for broken markdown links, not really for all links of bad markdown issues.

A missing image path/url does not really look related to a markdown link to me. Is it really a good idea to try to use this option for any markdown-related issue we have? Not sure. Do we also want another option just for missing image URLs? Not sure either.

What I know is that Docusaurus should nudge users to do the right thing, and have non-empty markdown link/image urls/paths seems appropriate.

If this is really important for you to support empty paths, why not building a remark plugin to support your specific usecase? You'd be able to tell the plugin to replace all empty tags by something else that we would allow, just as # or /img/placeholder.png

This should not be too complicated to write such logic, and we could even document this better on Docusaurus site?

Here's a pseudo implementation, didn't test but it shouldn't be much more complicated than that remark plugin:

const visit = require('unist-util-visit');
const path = require('path');

const plugin = () => {
  const transformer = (root) => {
    visit(root, 'link', (node) => {
      if ( !node.url ) {
        node.url = "#";
      }
    });
    visit(root, 'image', (node) => {
      if ( !node.url ) {
        node.url = "/img/placeholder.png";
      }
    });
  };
  return transformer;
};

module.exports = plugin;

sserrata commented 3 years ago

Hi @slorber, thanks so much for taking the time to respond and for sharing the plugin sample code - I will definitely give it a try. Thanks!

slorber commented 3 years ago

Please let me know how it works for you. Many users probably assume it's too complex to build such a remark plugin, and we should definitively highlight it's not complicated to build custom markdown syntax and document this better ;)

sserrata commented 3 years ago

Hi @slorber, I tested the remark plugin you suggested and I'm stilling seeing the same Error: Markdown link url is mandatory. and Error: Markdown Image url is mandatory. errors. Is it possible that the markdown URL check occurs before the plugin is able to parse and remediate the empty URLs?

slorber commented 3 years ago

@sserrata if you use the beforeDefaultRemarkPlugins your plugin should theoretically run before.

Having a repro would be easier for me to be able to help you.

I think I got it working in codesandbox: https://codesandbox.io/s/trusting-firefly-ku6ym?file=/docusaurus.config.js (but currently in 502 http status 😅 )

sserrata commented 3 years ago

Hi @slorber, thanks for that tip. The build seems to be getting further along with beforeDefaultRemarkPlugins but I am still seeing errors like:

Error: Image static/doc_imgs/reference/iam_configuration.png used in docs/reference/articles/identity-lifecycle-management.md not found.

I currently have onBrokenMarkdownLinks set to "warn" - should it apply to markdown image URLs as well or am I missing something?

slorber commented 3 years ago

@sserrata as I said onBrokenMarkdownLinks only applies to markdown links, not image paths.

We could as well add a onBrokenMarkdownImagePaths, not sure it's good to reuse the existing setting as it's not clear it applies to images (an image is not a link)?

I'm ok to add more setting to customize these error cases and make them fail-safe but we need some additional docusaurus infra to apply these settings more reliably to all plugins (not just docs) without duplicating code.

Note you can still hardcode a special case in your remark plugin to handle this specific broken image:

const visit = require('unist-util-visit');
const path = require('path');

const plugin = () => {
  const transformer = (root) => {
    visit(root, 'image', (node) => {
      if ( !node.url ) {
        node.url = "/img/placeholder.png";
      }
      if (node.url === "static/doc_imgs/reference/iam_configuration.png") {
             node.url = "/img/placeholder.png";
       }
    });
  };
  return transformer;
};

module.exports = plugin;

It won't prevent future commits to break the site but at least you have a way to upgrade without changing your docs. But it would be better to actually fix your existing docs broken images.

sserrata commented 3 years ago

Hi @slorber , our team would certainly be interested in seeing support for onBrokenMarkdownImagePaths added to D2.

facebook / docusaurus