Fix to add _missingMdxReference in some more cases

[vlad-zhukov]

While working on #1986 I found that _missingMdxReference checks are not injected sometimes. For example:

export const a = {}

<a.b />

compiles to

/*@jsxRuntime automatic @jsxImportSource react*/
const {jsx: _jsx} = arguments[0];
const a = {};
function MDXContent(props = {}) {
  const {wrapper: MDXLayout} = props.components || ({});
  return MDXLayout ? _jsx(MDXLayout, Object.assign({}, props, {
    children: _jsx(_createMdxContent, {})
  })) : _createMdxContent();
  function _createMdxContent() {
    return _jsx(a.b, {});
  }
}
return {
  a,
  default: MDXContent
};

but if I add some markdown content

Hello, world!

export const a = {}

<a.b />

compiles to

/*@jsxRuntime automatic @jsxImportSource react*/
const {Fragment: _Fragment, jsx: _jsx, jsxs: _jsxs} = arguments[0];
const a = {};
function MDXContent(props = {}) {
  const {wrapper: MDXLayout} = props.components || ({});
  return MDXLayout ? _jsx(MDXLayout, Object.assign({}, props, {
    children: _jsx(_createMdxContent, {})
  })) : _createMdxContent();
  function _createMdxContent() {
    const _components = Object.assign({
      p: "p"
    }, props.components);
    if (!a) _missingMdxReference("a", false);
    if (!a.b) _missingMdxReference("a.b", true);
    return _jsxs(_Fragment, {
      children: [_jsx(_components.p, {
        children: "Hello, world!"
      }), "\n", "\n", _jsx(a.b, {})]
    });
  }
}
return {
  a,
  default: MDXContent
};
function _missingMdxReference(id, component) {
  throw new Error("Expected " + (component ? "component" : "object") + " `" + id + "` to be defined: you likely forgot to import, pass, or provide it.");
}

This happens because all transforms are inside this if statement.

if (defaults.length > 0 || actual.length > 0) {

I fixed it by moving code that injects _missingMdxReference statements outside of this if.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mdx/mdx/CtWLCuJ3McXBxHJJSFcAoWZVPqjL
✅ Preview: https://mdx-git-fork-vlad-zhukov-patch-1-mdx.vercel.app

[codecov-commenter]

Codecov Report

Merging #1988 (bf762c8) into main (b8a76c9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1988   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2268      2270    +2     
=========================================
+ Hits          2268      2270    +2     

Impacted Files	Coverage Δ
packages/mdx/lib/plugin/recma-jsx-rewrite.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8a76c9...bf762c8. Read the comment docs.

[wooorm]

Thanks, the code seems good, but can you change the tests to instead add tests that run MDX, which should/should not throw errors and warnings, instead of a certain format of generated code?

Some existing tests for the behavior you’re changing are here:

mdx/packages/mdx/test/compile.js

Lines 483 to 540 in cd9b32f

	try {
	renderToStaticMarkup(React.createElement(await run(compileSync('<X />'))))
	assert.unreachable()
	} catch (/** @type {unknown} */ error) {
	const exception = /** @type {Error} */ (error)
	assert.match(
	exception.message,
	/Expected component `X` to be defined/,
	'should throw if a required component is not passed'
	)
	}
	try {
	renderToStaticMarkup(React.createElement(await run(compileSync('<a.b />'))))
	assert.unreachable()
	} catch (/** @type {unknown} */ error) {
	const exception = /** @type {Error} */ (error)
	assert.match(
	exception.message,
	/Expected object `a` to be defined/,
	'should throw if a required member is not passed'
	)
	}
	try {
	renderToStaticMarkup(
	React.createElement(await run(compileSync('<X />', {development: true})))
	)
	assert.unreachable()
	} catch (/** @type {unknown} */ error) {
	const exception = /** @type {Error} */ (error)
	assert.match(
	exception.message,
	/It’s referenced in your code at `1:1-1:6/,
	'should pass more info to errors w/ `development: true`'
	)
	}
	try {
	renderToStaticMarkup(
	React.createElement(
	await run(
	compileSync(
	{value: 'asd <a.b />', path: 'folder/example.mdx'},
	{development: true}
	)
	)
	)
	)
	assert.unreachable()
	} catch (/** @type {unknown} */ error) {
	const exception = /** @type {Error} */ (error)
	assert.match(
	exception.message,
	/It’s referenced in your code at `1:5-1:12` in `folder\/example.mdx`/,
	'should show what file contains the error w/ `development: true`, and `path`'
	)
	}

.

[vlad-zhukov]

@wooorm Done

[wooorm]

Thanks!

• One big question
• Two tips on improved test messages!

[wooorm]

Wait, why is this error thrown here? Is render being called?

[vlad-zhukov]

It does. That's exactly the issue I fix in #1986. This line is added:

if (!x) _missingMdxReference("x", false);',

But x is not there, it's defined inside an arrow function.

[vlad-zhukov]

Sorry I didn't communicate it properly, but both these errors will be gone in #1986.

[wooorm]

Ahh, okay.

If these two tests test broken behavior that will be fixed in the next PR, then:

• Can you add a small comment above them for now 😅 // To do this is wrong, will be fixed in GH-123123.
• Can you add actual tests for what is broken right now, and will be fixed by your changes to lib/?

Furthermore, for the next PR, I believe the first error should remain an error? Because b is not defined in a!

[vlad-zhukov]

Can you add actual tests for what is broken right now

I'm not sure how to do it. This PR fixes an edge case so two code paths produce the same code now.

Furthermore, for the next PR, I believe the first error should remain an error? Because b is not defined in a!

Oh, right! But it will be a React error because rendering undefined is illegal

[wooorm]

This PR fixes an edge case so two code paths produce the same code now.

But there was an error in one of those two though, right?

Oh, right! But it will be a React error because rendering undefined is illegal

Taking from the code in this issue:

if (!a.b) _missingMdxReference("a.b", true);

If possible, that should eventually be the error.

[vlad-zhukov]

The first test covers it, isn't it?

[wooorm]

The first test covers it, isn't it?

I thought so but then you said it didn’t?:

but both these errors will be gone in #1986.

---

To recap:

• If there are tests that are about to change, please add a little comment next to those tests saying that they’re incorrect and will be fixed.
• If there are only incorrect tests, please also add an actual, working, correct test

[wooorm]

OK I’ll merge this but I think the first test is the way it should be if possible. Thanks!

[vlad-zhukov]

Thanks!

[wooorm]

Released in 2.1.1!

[vlad-zhukov]

Thank you for the quick responses and your work on this great project!