Issue#20000 markdown caption error for remarks image

[shan-hua]

About the issue: #20000
It is caused by using the async function in compiler.generateHTML(). Since the caption written in Markdown will not be converted into a node, I changed it into using unified parser directly. It works but maybe it affords a better solution..

[shan-hua]

Related to this PR: #16574

[shan-hua]

Hi! Any feedbacks from the owner? :)

[LekoArts]

Hi, thanks for the PR!

This plugin option seems a bit convoluted as there is also this PR: #21188

I suppose this wouldn't fix #2000 correct?

---

Can you please add tests to make sure that the intended behavior is working with this change and we don't see any regressions in the future? Thanks!

[shan-hua]

Hi @LekoArts , thank you for the reply! I checked the code in #21188, I've not tried or tested it but I guess it was a solution to solve the issue #20000 using mdx.sync, but the mdx plugin is required and I (and I think there are others) don't use mdx in the product.

I'll add tests to it once I have time!

[ornotandrew]

I've done more digging and I'm now convinced that the internal property is the real problem. I'm now in agreement with @shan-hua that this PR is a good way to solve it. Apologies for my previous long message - I clearly didn't understand the code well enough.

I've included my justification for this below.

---

The functions in gatsby-transformer-remark/extend-node-type.js are written expecting markdownNode to look like this:

const markdownNode = {
  id: createNodeId(`${node.id} >>> MarkdownRemark`),
  children: [],
  parent: node.id,
  internal: {
    content: data.content,
    type: `MarkdownRemark`,
  },
}

Normally, they seem to be invoked with objects created in the normal way.

I think this problem started with gatsby-remark-images using the new compiler option from PR #16574 incorrectly.

My limited understanding here is:

compiler: {
  parseString: remark.parse.bind(remark),     // <---- The output of this
  generateHTML: getHTML                       // <---- Cannot be chained into this
},

And that passing the compiler option down is probably overcomplicating this feature. I think using the parsing libs directly makes more sense.

[ornotandrew]

@LekoArts There is already a full suite of tests for this feature. Previously, the tests were passing because the compiler option was mocked out incorrectly (the mock does exactly what this PR is doing and uses the parsing libs directly). The discrepancy between the mock and production code was a real issue.

This PR now causes the tests to pass correctly.

I think the only possible change to the tests might be to remove this line, since it's no longer used. I've tried this locally and the tests still pass.

---

Perhaps we could get this PR merged so long, and open up a different discussion about the compiler option?

[vladar]

I guess the idea is that gatsby-remark-images must be compatible with both - gatsby-transformer-remark AND gatsby-plugin-mdx.

This fix will make it work with gatsby-transformer-remark but not with gatsby-plugin-mdx.

@mathieudutour What do you think on this?

[mathieudutour]

Yeah, the idea was that gatsby-remark-images is given the compiler so that it can be used anywhere.

This fix is hardcoding hast-util-to-html and so:

• won't work with mdx
• won't use the other remark plugins

I think the proper fix is to make getImageCaption async and await the compiler

[vladar]

So ideally the issue should be fixed in #21188. I label this PR as blocked, for now, to make sure it is not merged prematurely. We'll circle back here after #21188 is resolved.

[pvdz]

• Confirmed the current implementation is broken due to async. The compiler call is async and the calling function (that's changed in this PR) is using the return value directly in jsx, except the return value must be a promise, so this will fail.
• The compiler call is async for two reasons;
  • Some loading stuff. and
  • It runs userland plugins, which may be async
• The proposed change in this PR does not run plugins and skip certain caches
• The linked PR adds hast-util-to-html and hast-util-to-html as devDependencies but I think those must now be regular dependencies as they're no longer "just" used in tests

This PR will be subsumed by #21188 and is actively being worked on. Considering this issue has been open since December, I don't think an extra week delay for the fix is a big deal.

As such I'm going to close this PR in favor and anticipation of merging #21188

@shan-hua Thank you for submitting the PR. Please don't feel discouraged for this closure! Your PR attempted to fix a bug in the codebase and we're grateful for it. If the issue is not fixed by the time #21188 lands, or if #21188 turns out to take much longer to land than "just a week", please reopen this and we'll try to get it merged as a workaround.

[shan-hua]

• Confirmed the current implementation is broken due to async. The compiler call is async and the calling function (that's changed in this PR) is using the return value directly in jsx, except the return value must be a promise, so this will fail.

• The compiler call is async for two reasons;

  • Some loading stuff. and
  • It runs userland plugins, which may be async

• The proposed change in this PR does not run plugins and skip certain caches

• The linked PR adds hast-util-to-html and hast-util-to-html as devDependencies but I think those must now be regular dependencies as they're no longer "just" used in tests

This PR will be subsumed by #21188 and is actively being worked on. Considering this issue has been open since December, I don't think an extra week delay for the fix is a big deal.

As such I'm going to close this PR in favor and anticipation of merging #21188

@shan-hua Thank you for submitting the PR. Please don't feel discouraged for this closure! Your PR attempted to fix a bug in the codebase and we're grateful for it. If the issue is not fixed by the time #21188 lands, or if #21188 turns out to take much longer to land than "just a week", please reopen this and we'll try to get it merged as a workaround.

Thank you very much! Sorry I've been completely out of Github community for workload reasons in the last two months. As I mentioned in the quick PR that there should be a better solution on the async issue which I could not be familiar with :) I already see there are updates regarding to #21188 and I'll have a look right away! Happy to continue working with Gatsby team