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

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

Closed
4 tasks done
bholmesdev opened this issue Aug 11, 2022 · 7 comments · Fixed by #2101
Closed
4 tasks done

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

bholmesdev opened this issue Aug 11, 2022 · 7 comments · Fixed by #2101
Labels
💪 phase/solved Post is done

Comments

@bholmesdev
Copy link
Contributor

bholmesdev commented Aug 11, 2022

Initial checklist

Affected packages and versions

^2.1.2"

Link to runnable example

https://stackblitz.com/github/bholmesdev/mdx-shiki-compilation?file=test.mjs

Steps to reproduce

  1. Run the MDX compiler with the rehypeRaw plugin applied + some other plugin that inserts a custom element (Shiki twoslash in this case).
  2. Set jsx: true in the compiler config
  3. Note the value contains <_components.custom-element-name> in the output, but dashes (-) are not valid JSX!

Expected behavior

Ideally, the _components statement would serialize dashes to camelCase or some other JSX-compliant string like so:

const _components = Object.assign({
    ...
    "customElementName": "custom-element-name"
  }, props.components);

Actual behavior

Instead, _components uses the element name untouched:

const _components = Object.assign({
   ...
    "custom-element-name": "custom-element-name"
  }, props.components);

Runtime

Node v16

Package manager

pnpm

OS

macOS

Build and bundle tools

Other (please specify in steps to reproduce)

@bholmesdev
Copy link
Contributor Author

This is related to a new issue in Astro's MDX integration: withastro/astro#4258

@wooorm
Copy link
Member

wooorm commented Aug 11, 2022

Could you get away with ditching the JSX language? Compiled away JSX can express this.

Otherwise, I'm not sure what MDX could do about it. Turning dashes into camel seems like something other people would disagree with.
We could turn off looking them up into "components" when JSX is turned on?
Or, maybe as a plugin or an option?

@bholmesdev
Copy link
Contributor Author

bholmesdev commented Aug 11, 2022

@wooorm See my PR above! Unfortunately, Astro relies on our own JSX transformation process, so we cannot abandon this JSX output. I also discovered a unit test in the repo that asserts MDX outputs invalid JSX for custom elements. Regardless, I think a fix is warranted.

I also agree that camel case isn't the best approach. I've opted for replacing invalid identifier characters with underscores _ instead! See PR for more.

@wooorm
Copy link
Member

wooorm commented Aug 11, 2022

One more alt: what if we turn invalid IDS into valid, different IDS, that still get them from components?

With a:

const _component1 = _components["a-b"]

...

<_component1>...

@bholmesdev
Copy link
Contributor Author

@wooorm That could work too! So create variables below the _components declaration with an incrementing counter?

@wooorm
Copy link
Member

wooorm commented Aug 11, 2022

Yeah, create throwaway variables.
Only create them if JSX is on (or always if it doesn't hurt, I don't think it typically does)
Counters are easiest, alternative and more readable is a camel case variant, with a tiny chance of conflicting.
If we only do it for invalid IDs I don't think it can ever conflict?

I like that much better than changing what we get from components, and hence that users have to change what they pass too?

@bholmesdev
Copy link
Contributor Author

@wooorm Addressed in the latest! Should be good for review. #2101

wooorm pushed a commit that referenced this issue Aug 17, 2022
This commit particularly solves elements names that include dashes: custom
elements, such as `custom-element-name`.
These can be written in JSX like so:

```jsx
<custom-element-name />
```

…which is fine.

But to support passing them in, we were previously rewriting such code to a
member id, to take a key from an object, like so:

```jsx
<_components.custom-element-name>
```

…which crashed.

This commit solves that by taking the component from the object in a temporary
variable, and using that valid variable name as a single component name.

```js
const _component0 = _components['custom-element-name']

<_component0 />
```

Closes GH-2100.
Closes GH-2101.

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

Reviewed-by: Titus Wormer <tituswormer@gmail.com>
@wooorm wooorm added the 💪 phase/solved Post is done label Aug 17, 2022
wooorm added a commit that referenced this issue Oct 11, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging a pull request may close this issue.

2 participants