fix(gatsby-plugin-mdx): pass the correct options to setParserPlugins

[d4rekanguok]

Description

Hey folks!

In gatsby-plugin-mdx, when setParserPlugins is called, it was passed in the wrong options ^source:

for (let plugin of options.gatsbyRemarkPlugins) {
  ...
    for (let parserPlugin of requiredPlugin.setParserPlugins(
        plugin.pluginOptions   // <--- undefined
      )) { ... }

Instead of plugin.pluginOptions, it should have been plugin.options.

Reproduction of bug

https://github.com/d4rekanguok/gatsby-bug-repro-18243

[pieh]

Seems like this is pretty much copy & paste from remark plugin -

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

Lines 128 to 142 in 68dbb6d

	for (let plugin of pluginOptions.plugins) {
	const requiredPlugin = require(plugin.resolve)
	if (_.isFunction(requiredPlugin.setParserPlugins)) {
	for (let parserPlugin of requiredPlugin.setParserPlugins(
	plugin.pluginOptions
	)) {
	if (_.isArray(parserPlugin)) {
	const [parser, options] = parserPlugin
	remark = remark.use(parser, options)
	} else {
	remark = remark.use(parserPlugin)
	}
	}
	}
	}

Which means if that's a bug we have it in both mdx and remark.

Do you have any reproduction so we could verify this change quickly?

[d4rekanguok]

Hi @pieh! I actually discovered this while debugging a remark plugin that worked with gatsby-transformer-remark but not gatsby-plugin-mdx, so I think the issue only exists with mdx.

I can quickly make & share a reproduction repo in an hour or so

[d4rekanguok]

@pieh I've just put this repro together:

https://github.com/d4rekanguok/gatsby-bug-repro-18243

The step to reproduce is detailed in the readme.

I think it might have something to do with the fact that remark plugins for mdx are passed in via options as gatsbyRemarkPlugins, while with remark it's plugins. I'm guessing Gatsby might have a recursive system that store plugin options as pluginOptions, but only recognize plugins set via the plugins property?

[pieh]

think it might have something to do with the fact that remark plugins for mdx are passed in via options as gatsbyRemarkPlugins, while with remark it's plugins. I'm guessing Gatsby might have a recursive system that store plugin options as pluginOptions, but only recognize plugins set via the plugins attribute?

Oh yeah, that would explain the difference.

You are correct - plugins field in options does have special handling, and we do some transformation, so if gatsbyRemarkPlugins is used, this transformation won't be done.

[d4rekanguok]

Oh in that case, instead of fixing this like I did in this PR, maybe it's better to also handle gatsbyRemarkPlugins specially to keep the plugin chain going?

This might be a really edge case however, since gatsby-plugin-mdx has been released for a long time & no one has yet filed a bug on this

[pieh]

I think this is fine. Other subplugin API use options instead of pluginOptions in mdx, so I think this is fine for now.

mdx (using plugin.options):

gatsby/packages/gatsby-plugin-mdx/utils/get-source-plugins-as-remark-plugins.js

Lines 53 to 73 in b4e3c5e

	debug(`userPlugins: constructing remark plugin for `, plugin)
	const requiredPlugin = require(plugin.resolve)
	const wrappedPlugin = () =>
	async function transformer(markdownAST) {
	await requiredPlugin(
	{
	markdownAST,
	markdownNode: mdxNode,
	getNode,
	files: fileNodes,
	pathPrefix,
	reporter,
	cache,
	},
	plugin.options || {}
	)
	return markdownAST
	}
	return [wrappedPlugin, {}]

remark (using plugin.pluginOptions):

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

Lines 216 to 235 in b4e3c5e

	const requiredPlugin = require(plugin.resolve)
	if (_.isFunction(requiredPlugin)) {
	return requiredPlugin(
	{
	markdownAST,
	markdownNode,
	getNode,
	files: fileNodes,
	basePath,
	reporter,
	cache: getCache(plugin.name),
	getCache,
	compiler: {
	parseString: remark.parse.bind(remark),
	generateHTML: getHTML,
	},
	...rest,
	},
	plugin.pluginOptions
	)

[d4rekanguok]

A few of my PRs is failing this unit_tests_node12:

FAIL peril/tests/validate-yaml/validate-yaml-starters.test.ts
● a new PR › Doesn't allow non GitHub repos

I'm not submitting new starters though, so I'm not sure why it's failing... any ideas? @pieh

[pieh]

I'm not submitting new starters though, so I'm not sure why it's failing... any ideas? @pieh

Oh yeah, this has something with recent Node 12 release (I think) - we "fixed" this (or rather workarounded it) in #18247, so I'll just merge master in this PR and that should fix that failing test

[pieh]

Verified fix. Works like a charm!

Huge thanks for reproduction making it much faster for us to merge this in!

[pieh]

Published gatsby-plugin-mdx@1.0.49