Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override jsx built-in components #2050

Closed
wants to merge 2 commits into from

Conversation

jlaramie
Copy link

I've been investigating the differences between mdx-bundler and next-remote-mdx@3.0.5 and discovered that no matter what I did mdx-bundler would not replace any jsx that had a native version if explicitly used inside of the mdx file.

Here is an example which shows 3 different scenarios and the result using latest:

<div>
  does not override: <img src="test.png" />
  <br />
  does override: ![Image](test.png)
  <br />
  does override: <Img src="test.png" />
</div>
  • Scenario 1: does not override: <img src="test.png" /> Since this is a built-in component, no override check is performed and jsxRuntime.jsx.img is used
  • Scenario 2: does override: ![Image](test.png) All md conversions automatically go through the components override check
  • Scenario 3: <Img src="test.png" /> Custom components automatically go through the components check

Eventually I tracked the difference between handling in mdx-js/mdx between version 1.*.* and 2.*.*.

Code Removed

// @ts-expect-error Allow fields passed through from mdast through hast to
// esast.
else if (node.data && node.data._mdxExplicitJsx) {
// Do not turn explicit JSX into components from `_components`.
// As in, a given `h1` component is used for `# heading` (next case),
// but not for `<h1>heading</h1>`.

The only reasoning I could come up with is that maybe it compiles slightly smaller and runs slightly faster but I don't think it'll have any real performance differences.

kentcdodds/mdx-bundler#160

@vercel
Copy link

vercel bot commented May 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
mdx ✅ Ready (Inspect) Visit Preview May 28, 2022 at 9:18PM (UTC)

@codecov-commenter
Copy link

Codecov Report

Merging #2050 (643e7e5) into main (63fd208) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main     #2050   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2308      2301    -7     
=========================================
- Hits          2308      2301    -7     
Impacted Files Coverage Δ
packages/mdx/lib/plugin/recma-jsx-rewrite.js 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 63fd208...643e7e5. Read the comment docs.

@ChristianMurphy
Copy link
Member

This behavior is intentional see the discussion in #821

@wooorm
Copy link
Member

wooorm commented May 29, 2022

This behavior is intentional

@wooorm wooorm closed this May 29, 2022
@wooorm wooorm added 🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on labels May 29, 2022
@jlaramie
Copy link
Author

jlaramie commented May 29, 2022

@ChristianMurphy @wooorm should disableParentContext get passed into rcma-jsx-rewrite.js? What I'm trying to figure out is how to get @mdx-js/esbuild to be able to work the same way @mdx-js/react works.

Something like this perhaps?

diff --git a/node_modules/@mdx-js/mdx/lib/core.js b/node_modules/@mdx-js/mdx/lib/core.js
index 96e34d7..19d0d6d 100644
--- a/node_modules/@mdx-js/mdx/lib/core.js
+++ b/node_modules/@mdx-js/mdx/lib/core.js
@@ -75,6 +75,7 @@ export function createProcessor(options = {}) {
     remarkPlugins,
     remarkRehypeOptions = {},
     SourceMapGenerator,
+    disableParentContext,
     ...rest
   } = options
   let index = -1
@@ -122,7 +123,7 @@ export function createProcessor(options = {}) {
   pipeline
     .use(rehypeRecma)
     .use(recmaDocument, {...rest, outputFormat})
-    .use(recmaJsxRewrite, {development, providerImportSource, outputFormat})
+    .use(recmaJsxRewrite, {development, providerImportSource, outputFormat, disableParentContext})
 
   if (!jsx) {
     pipeline.use(recmaJsxBuild, {outputFormat})
diff --git a/node_modules/@mdx-js/mdx/lib/plugin/recma-jsx-rewrite.js b/node_modules/@mdx-js/mdx/lib/plugin/recma-jsx-rewrite.js
index e1d8dbf..5a6c7a8 100644
--- a/node_modules/@mdx-js/mdx/lib/plugin/recma-jsx-rewrite.js
+++ b/node_modules/@mdx-js/mdx/lib/plugin/recma-jsx-rewrite.js
@@ -59,7 +59,8 @@ const own = {}.hasOwnProperty
  * @type {import('unified').Plugin<[RecmaJsxRewriteOptions]|[], Program>}
  */
 export function recmaJsxRewrite(options = {}) {
-  const {development, providerImportSource, outputFormat} = options
+  const {development, providerImportSource, outputFormat, disableParentContext} = options
 
   return (tree, file) => {
     // Find everything that’s defined in the top-level scope.
@@ -183,7 +184,7 @@ export function recmaJsxRewrite(options = {}) {
           }
           // @ts-expect-error Allow fields passed through from mdast through hast to
           // esast.
-          else if (node.data && node.data._mdxExplicitJsx) {
+          else if (!disableParentContext && node.data && node.data._mdxExplicitJsx) {
             // Do not turn explicit JSX into components from `_components`.
             // As in, a given `h1` component is used for `# heading` (next case),
             // but not for `<h1>heading</h1>`.

@jlaramie
Copy link
Author

Also while messing with the tests it's unclear to me that disableParentContext as it is currently implemented fixes this issue.

disableParentContext seems to only control whether markdown tags use the provider or not but have no control over whether native elements can be overridden.

@wooorm
Copy link
Member

wooorm commented May 29, 2022

I do not believe disableParentContext has anything to do with what you want: it has no relation to your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

4 participants