feat(gatsby-remark-images): support markdownCaptions in mdx + fix for remark

[mathieudutour]

Description

Second part of #16574. In the previous PR, we pass down a compiler (in the gatsby-transformer-remark package) so that gatsby-remark plugins can use it to recursively parse a piece of content (in that case, the caption of an image). But when used in the gatsby-plugin-mdx, the remark plugins didn't get the compiler.
This PR fixes it.

Related Issues

Second part of #16574.
Fixes #14474 for the MDX case
Fixes #20000

[mathieudutour]

can't seem to be able to see why the build service failed

[mathieudutour]

we really should move this to TS. Happy to take on that if nobody started those packages yet

[vladar]

Unfortunately, it didn't fix the #20000 issue. There is more about it:

gatsby/packages/gatsby-transformer-remark/src/extend-node-type.js

Lines 343 to 346 in ca19076

	async function getHTML(markdownNode) {
	const shouldCache = markdownNode && markdownNode.internal
	const cachedHTML =
	shouldCache && (await cache.get(htmlCacheKey(markdownNode)))

As you see getHTML accepts Gatsby node, not markdown AST. So when you feed it with the result of parseString from here:

gatsby/packages/gatsby-transformer-remark/src/extend-node-type.js

Lines 184 to 187 in ca19076

	compiler: {
	parseString: remark.parse.bind(remark),
	generateHTML: getHTML,
	},

It still fails with the same error:

UNHANDLED REJECTION Cannot read property 'contentDigest' of undefined

Because remark.parse returns AST and getHTML expects gatsby node. I think you actually need to change compiler in gatsby-transformer-remark to:

compiler: { 
  parseString: remark.parse.bind(remark), 
  generateHTML: ast => hastToHTML(toHAST(ast)), 
}

(then it all can stay sync?). But I didn't test it.

[mathieudutour]

Oh you're right.

The issue with hastToHTML(toHAST(...)) is that you bypass all the remark plugins and the caches. So I'd be more tempted to change remark.parse.bind(remark) to something like

// returns a node-like object
(string) => ({
  internal: {
    contentDigest: digest(string),
    content: remark.parse(string)
  }
})

[vladar]

This looks like a hack that will one day shoot us in the foot (this happens more often than not).

Also how much benefit do we get from cache here? I doubt that it will be a significant boost (as far as I understand we don't have cache in MDX version anyway?)

If we still want cache we can just add a separate function similar to getHTML which will cache this (it should be simple enough to write).

Does it make sense?

[mathieudutour]

It's mostly about the plugins and options for the users that would be ignored (might be really confusing if some part of the MD is parsed with some plugins while others aren't). I'm actually not too worried about the caches because the captions will always be small.

But I had it all wrong, not sure what I did last time. The issue is not about generateHTML but about parseString. Could you check my latest commit?

[vladar]

My best suggestion so far would be to exclude mutateSource part from parsing.

In other words, maybe worth extracting method parseString(string, markdownNode) from this piece:

gatsby/packages/gatsby-transformer-remark/src/extend-node-type.js

Lines 196 to 248 in 4043129

	const markdownAST = remark.parse(markdownNode.internal.content)
	if (basePath) {
	// Ensure relative links include `pathPrefix`
	visit(markdownAST, [`link`, `definition`], node => {
	if (
	node.url &&
	node.url.startsWith(`/`) &&
	!node.url.startsWith(`//`)
	) {
	node.url = withPathPrefix(node.url, basePath)
	}
	})
	}
	if (process.env.NODE_ENV !== `production` || !fileNodes) {
	fileNodes = getNodesByType(`File`)
	}
	// Use Bluebird's Promise function "each" to run remark plugins serially.
	await Promise.each(pluginOptions.plugins, plugin => {
	const requiredPlugin = require(plugin.resolve)
	// Allow both exports = function(), and exports.default = function()
	const defaultFunction = _.isFunction(requiredPlugin)
	? requiredPlugin
	: _.isFunction(requiredPlugin.default)
	? requiredPlugin.default
	: undefined
	if (defaultFunction) {
	return defaultFunction(
	{
	markdownAST,
	markdownNode,
	getNode,
	files: fileNodes,
	basePath,
	reporter,
	cache: getCache(plugin.name),
	getCache,
	compiler: {
	parseString: remark.parse.bind(remark),
	generateHTML: getHTML,
	},
	...rest,
	},
	plugin.pluginOptions
	)
	} else {
	return Promise.resolve()
	}
	})
	return markdownAST

And use this new method in both getMarkdownAST and in your compiler.parseString. Then mutateSource will be applied for the whole node once (in getMarkdownAST) and remark plugins will still work for captions parsing.

[pvdz]

(You can ignore gatsby

[pvdz]

Sorry. I was going to say that you can ignore the "Gatsby Build Service" failures as they are unrelated to this PR and they are non-blocking. Then mid-way I managed to activate the close button :/ So anyways. Ignore those failures.

[vladar]

Ok, looks good to me now! Thanks for this work! Let's ship it 🚢

[vladar]

Published in:

• gatsby-plugin-mdx@1.1.10
• gatsby-remark-images@3.2.5
• gatsby-transformer-remark@2.7.4

[afc163]

This breaks https://github.com/rstacruz/gatsby-remark-component

@rstacruz

[mathieudutour]

how so?

[chris-erickson]

Now when I enable markdownCaptions: true I'm getting an unhandled promise rejection. The showCaptions: ['title'] option does work if markdownCaptions: false (but the MD is unprocessed!). How can I debug this particular issue?

[mathieudutour]

1. What the error of the rejected promise?
2. What's your config?
3. Do you have a repro somewhere?

[chris-erickson]

So sorry, I forgot to include the error: UNHANDLED REJECTION Expected node, not '[object Promise]'

gatsby-config.json snippet.

    {
      resolve: `gatsby-transformer-remark`,
      options: {
        commonmark: true,
        footnotes: true,
        pedantic: true,
        gfm: true,
        plugins: [
          {
            resolve: `gatsby-remark-images`,
            options: {
              maxWidth: 960,
              quality: 90
              showCaptions: ['title', 'alt'],
              markdownCaptions: true,
              linkImagesToOriginal: false,
            },
          },
        ],
      },
    },

I don't have a reproduction at the moment, our site is quite expansive so it would take me a bit to do that. If this doesn't provide any useful leads, I can do that. FWIW, this seems to fail regardless of whether I have a "title" attribute in a MD image link, in other words, it seems like it's not a parsing issue at first glance.

[mathieudutour]

found it: #23427