Fix to not add _missingMdxReference when thing is in scope

[vlad-zhukov]

When using object members as components, for example <x.y />, MDX always adds the following checks:

if (!x) _missingMdxReference("x", false);
if (!x.y) _missingMdxReference("x.y", true);

It works for basic cases, but breaks when used inside a function expression and x is assigned locally:

Hello, world!

{(() => {
    const x = { y: () => '' }
    return <x.y />
})()}

This example throws x is not defined error because the variable is locally scoped. The solution I propose is to add _missingMdxReference only when an object (in this case x) is not in scope.

[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/DnpX3Y7MyivdP5HadxHySwah7Wqd
✅ Preview: https://mdx-git-fork-vlad-zhukov-fix-dont-add-object-check-w-22543a-mdx.vercel.app

[wooorm]

Can you add tests for this behavior?

[vlad-zhukov]

I already did, it's the same as in my PR message and it's the smallest one I could think of.

But now I think a more real example would be the one with render props. Maybe something like this?

<a.b render={(c) => <c.d />} />

[wooorm]

Oh huh I missed it sorry.

It’s better to not test the serialized output. Your change fixes an exception. So can you add a “normal” test, like all the other ones outside of “jsx”?

• In your existing test, please try and remove everything that’s not needed for the error to occur
• In your newly proposed test, the a.b is also not needed?
• What about say the following, or other possible tests?:

  export const a = {}
  
  <a.b />

Lastly, the code is a bit difficult, but I believe your proposed change change the place that tracks references. These components and objects are still referenced? Could we instead improve where and how _missingMdxReference is injected?

[wooorm]

Also what was your practical MDX (so not the minimal test, but the original) that firs resulted in this problem?

[vlad-zhukov]

In your newly proposed test, the a.b is also not needed?

Correct, it doesn't

What about say the following, or other possible tests?:

It would work. But I found another issue that if there are no markdown components, _missingMdxReference is not injected 🤷 Thats why I added Hello, world! in my example

Lastly, the code is a bit difficult, but I believe your proposed change change the place that tracks references. These components and objects are still referenced? Could we instead improve where and how _missingMdxReference is injected?

As far as I understand, these references are only used for injecting _missingMdxReference. And in the next case this check is there:

https://github.com/mdx-js/mdx/blob/427bcad073b36637b250ba7dffe1b33f75c003ec/packages/mdx/lib/plugin/recma-jsx-rewrite.js#L155:L169

Also what was your practical MDX (so not the minimal test, but the original) that firs resulted in this problem?

The original code with which I discovered this problem is not far from the minimal test. Something along the lines:

<CodePreview preview={() => {
  const store = createStore();
  return <store.Provider>...</store.Provider>;
}} />

[vlad-zhukov]

See #1988. I will update this PR after that one.

[wooorm]

is this ready for review?

[vlad-zhukov]

It's ready. I found a way to make the first test case remain as it was. What do you think about this approach?

[wooorm]

Sweet, some style notes, otherwise great!

[wooorm]

Thanks! :)

[wooorm]

Released in 2.1.1!