Fix rewriting of components for custom elements #2101

bholmesdev · 2022-08-11T23:39:28Z

Resolves #2100

Writes invalid component identifiers to separate variables, with naming _component[#]
Use separate variables instead of _components object when jsx: true

vercel · 2022-08-11T23:39:33Z

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

Name	Status	Preview	Updated
mdx	✅ Ready (Inspect)	Visit Preview	Aug 15, 2022 at 8:53PM (UTC)

packages/mdx/test/compile.js

codecov-commenter · 2022-08-11T23:43:48Z

Codecov Report

Merging #2101 (a9d93b2) into main (529b96a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #2101   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2009      2044   +35     
=========================================
+ Hits          2009      2044   +35

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

This reverts commit 135aa2d.

This reverts commit 389b5a5.

This reverts commit f1205fe.

This reverts commit 1c86a45.

This reverts commit 11b96d3.

wooorm

Looking good. One important thing. One small thing. One stylistic thing!

packages/mdx/lib/plugin/recma-jsx-rewrite.js

packages/mdx/test/compile.js

wooorm · 2022-08-14T21:55:27Z

packages/mdx/test/compile.js

      '}',
      'function MDXContent(props = {}) {',
-      '  const {wrapper: MDXLayout} = props.components || ({});',
+      '  const _component0 = "a-b", {wrapper: MDXLayout} = props.components || ({});',


Same here as above?

Ah, I think I understand this prop a bit better now. Looks like I didn't implement this right! I'll rework 👍

Sweet, thanks!
You could also likely set it on that destructuring object, alongside wrapper: MDXLayout? You may need to set a computed field in the AST or so though 🤔: https://astexplorer.net/#/gist/702f0315bf76ddbaf45a0928b0b06e96/784539f882835483b4541f9eff8d81ec4a327733

wooorm

Getting very close, thanks for your continued work! One big thing, rest are nits

packages/mdx/lib/plugin/recma-jsx-rewrite.js

wooorm · 2022-08-15T18:03:40Z

packages/mdx/lib/plugin/recma-jsx-rewrite.js

-            ] of invalidComponentNameToVariable) {
+              _componentsId,
+              _componentName
+            ] of idToInvalidComponentName) {


I still kinda like name, value better, but I’m not blocking on this.
I feel a bit more strongly about not using _ to start variable names, that is often used with variables (parameters) to mark them as explicitly not used

Understood, removed those _s! I also tried using name and value, but noticed it leads to {value: name} and {name: value} in the sections where they're used. It... kinda made my head spin, so I chose [id, componentName] instead 😆

packages/mdx/lib/plugin/recma-jsx-rewrite.js

packages/mdx/test/compile.js

Co-authored-by: Titus <tituswormer@gmail.com>

bholmesdev · 2022-08-15T20:57:12Z

@wooorm Alright, I think we finally nailed it. Ready for review 😁

wooorm · 2022-08-17T18:49:28Z

Released in 2.1.3!

bholmesdev · 2022-08-17T18:55:56Z

@wooorm Woo! Thanks for the thorough review and feedback here. Already learned a ton about the MDX project ❤️

This fixes a bug, uncovered in #2112 and in #2123, which is rather unlikely to occur. It only occurs when: 1. Custom elements (such as `<a-b></a-b>`) are injected (not authored) into a tree 2. Either: 1. A layout component is defined within an MDX document 2. A provider is used, and any component is defined within MDX documents In those cases, an accidental `const ;` was injected into the final serialized document. Which caused anything trying to run the code to crash. The problem was introduced in 9904838, the commit message of which sheds some light on why custom elements are peculiar and need extra handling. We track which component contains which other components. If some component uses `<A />`, then some code to handle `A` is injected in that component. If a different component uses `<B />`, some code for `B` is injected inside it. But the components don’t need to know about what’s used in other components. This mechanism had a mistake for custom elements: they were tracked globally. This commit fixes that, by tracking them scoped to the component that includes them. Related-to: GH-2100. Related-to: GH-2101. Closes GH-2112. Closes GH-2123. Co-authored-by: Caleb Eby <caleb.eby01@gmail.com> Co-authored-by: bholmesdev <hey@bholmes.dev>

bholmesdev added 5 commits August 11, 2022 19:18

feat: add toValidIdentifierName util

11b96d3

feat: serialize custom element component keys

1c86a45

chore: update custom element test case

f1205fe

fix: update JSdoc reference

389b5a5

test: to-valid-identifier-name

135aa2d

bholmesdev commented Aug 11, 2022

View reviewed changes

packages/mdx/test/compile.js Show resolved Hide resolved

vercel bot deployed to Preview August 11, 2022 23:41 View deployment

bholmesdev mentioned this pull request Aug 11, 2022

Usage of Shiki Twoslash within MDX crashes Astro with ERROR: Unexpected "-" withastro/astro#4258

Closed

1 task

bholmesdev added 7 commits August 12, 2022 16:07

Revert "test: to-valid-identifier-name"

0553e0c

This reverts commit 135aa2d.

Revert "fix: update JSdoc reference"

8f5fbe0

This reverts commit 389b5a5.

Revert "chore: update custom element test case"

a3bd1de

This reverts commit f1205fe.

Revert "feat: serialize custom element component keys"

2ec33fa

This reverts commit 1c86a45.

Revert "feat: add toValidIdentifierName util"

5878dd8

This reverts commit 11b96d3.

feat: inline variables for invalid comp names

451a76f

test: update custom elements

e547aa8

vercel bot deployed to Preview August 12, 2022 21:02 View deployment

bholmesdev mentioned this pull request Aug 12, 2022

jsx: true output does not handle custom elements correctly #2100

Closed

4 tasks

wooorm reviewed Aug 14, 2022

View reviewed changes

bholmesdev added 3 commits August 15, 2022 12:28

fix: avoid duplicating jsx expr

7572cdc

feat: change to "_componentX = _components['X']"

604e6db

test: update output

24fb34d

vercel bot deployed to Preview August 15, 2022 17:19 View deployment

wooorm reviewed Aug 15, 2022

View reviewed changes

bholmesdev and others added 3 commits August 15, 2022 16:39

nit: ...[] => Array<...>

d051f7c

Co-authored-by: Titus <tituswormer@gmail.com>

nit: clarify inline docs

f7aea66

Co-authored-by: Titus <tituswormer@gmail.com>

refactor: [id, componentName]

55414b4

vercel bot deployed to Preview August 15, 2022 20:44 View deployment

fix: only add _componentX to _createMdxContent

a9d93b2

vercel bot deployed to Preview August 15, 2022 20:53 View deployment

wooorm approved these changes Aug 17, 2022

View reviewed changes

wooorm changed the title ~~Fix custom element serialization for jsx: true~~ Fix rewriting of components for custom elements Aug 17, 2022

wooorm merged commit 9904838 into mdx-js:main Aug 17, 2022

wooorm added 🐛 type/bug This is a problem 🗄 area/interface This affects the public interface 💪 phase/solved Post is done labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rewriting of components for custom elements #2101

Fix rewriting of components for custom elements #2101

bholmesdev commented Aug 11, 2022 •

edited

vercel bot commented Aug 11, 2022 •

edited

codecov-commenter commented Aug 11, 2022 •

edited

wooorm left a comment

wooorm Aug 14, 2022

bholmesdev Aug 15, 2022

wooorm Aug 15, 2022

wooorm left a comment

wooorm Aug 15, 2022

bholmesdev Aug 15, 2022

bholmesdev commented Aug 15, 2022

wooorm commented Aug 17, 2022

bholmesdev commented Aug 17, 2022

Fix rewriting of components for custom elements #2101

Fix rewriting of components for custom elements #2101

Conversation

bholmesdev commented Aug 11, 2022 • edited

vercel bot commented Aug 11, 2022 • edited

codecov-commenter commented Aug 11, 2022 • edited

Codecov Report

wooorm left a comment

Choose a reason for hiding this comment

wooorm Aug 14, 2022

Choose a reason for hiding this comment

bholmesdev Aug 15, 2022

Choose a reason for hiding this comment

wooorm Aug 15, 2022

Choose a reason for hiding this comment

wooorm left a comment

Choose a reason for hiding this comment

wooorm Aug 15, 2022

Choose a reason for hiding this comment

bholmesdev Aug 15, 2022

Choose a reason for hiding this comment

bholmesdev commented Aug 15, 2022

wooorm commented Aug 17, 2022

bholmesdev commented Aug 17, 2022

bholmesdev commented Aug 11, 2022 •

edited

vercel bot commented Aug 11, 2022 •

edited

codecov-commenter commented Aug 11, 2022 •

edited