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

Compatibility with Solid JSX and other flavors #7

Closed
4 tasks done
DaniGuardiola opened this issue Feb 5, 2023 · 9 comments
Closed
4 tasks done

Compatibility with Solid JSX and other flavors #7

DaniGuardiola opened this issue Feb 5, 2023 · 9 comments
Labels
💪 phase/solved Post is done

Comments

@DaniGuardiola
Copy link
Contributor

Initial checklist

Problem

To set up MDX with Solid (e.g. Solid Start), I need to use @mdx-js/rollup which calls @mdx-js/mdx, which uses the rehypeRecma plugin, which calls this util. Then, this util has the element handler which takes care of creating the AST for the JSX elements.

The way it works seems tailored to React in some ways. The two problematic ones I've found are:

  • className prop: while it still works in Solid, it's been deprecated and will probably be removed at some point - ref: https://www.solidjs.com/docs/latest#classlist
  • style prop: this library turns inline CSS strings into CSS-in-JS syntax, which Solid does understand, but it does so in camel-case. Solid expects the properties to be in their original casing (e.g. background-color not backgroundColor), which makes the output of some rehype plugins incompatible - ref: ref: https://www.solidjs.com/docs/latest#classlist (again 😄)

There might be more instances of this, but I'm not sure.

Solution

I've seen that this library supports configuring the handlers, which is great. However, I have two thoughts about this:

  • Replacing the handler for just this one change would mean forking a huge file to replace logic that only spans ~10 lines of code, which is not ideal. Ideally, this would be configurable in a simpler way from within the handler itself.
  • This configuration would need to be available all the way up the chain, up to @mdx-js/rollup. I guess it could be a jsxSomething option (jsxFlavor? haha). This util it could have the same option and then it could be passed to the default handlers through the state (I guess? not super familiar so I can't tell if that makes sense).

Alternatives

A solution I've thought of is to create a rehype plugin that "undoes" these changes (e.g. renaming className to class and de-camelcasing the style objects). The problem is that rehypeRecma runs after the external plugins, so there's no way to cleanly execute this.

The only way would be to patch @mdx-js/mdx to introduce the plugin (in core.js), but if I'm patching the package, I might as well just patch this one (element handler) to prevent this from happening in the first place.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Feb 5, 2023
@wooorm
Copy link
Member

wooorm commented Feb 5, 2023

See #6

@DaniGuardiola
Copy link
Contributor Author

Oh, wow, this is great. Sorry, I missed it somehow. Thanks!

I was gonna give a quick update and say that className doesn't seem to be coming from this util though. It seems to be already coming from rehype-katex (the plugin I'm working with). I haven't looked for the specific source yet though.

@wooorm
Copy link
Member

wooorm commented Feb 5, 2023

No worries :)

The casing used in remark/rehype/etc is intentional. className is supposed to be like that.
You could change it in a recma plugin. But it can also be solved like #6

@DaniGuardiola
Copy link
Contributor Author

DaniGuardiola commented Feb 5, 2023

@wooorm I ended up investigating for another hour or two (it's been the whole night in total). I'm learning a lot about this whole ecosystem lmao.

Disclaimer: I have only skimmed through the PR, this is probably already covered by that discussion/code, but these are my findings.

I know now that className is supposed to be there for hast, following the DOM property convention.

However, this same handler is exporting the attribute as "className" as well, specifically in this part:

info.space
  ? hastToReact[info.property] || info.property
  : info.attribute

Since info.space will be "html" (truthy), and since the hastToReact lookup doesn't return anything, it will end up with the value of info.property, which is className.

I don't know enough to know what needs to happen here, but for now I've patched both of these issues like this:

diff --git a/lib/handlers/element.js b/lib/handlers/element.js
index f0718a28c971a1339a8a94bd38b80b91c3de4c6e..61b00b5823a8329a9207c2c3c5a216f61a6639e9 100644
--- a/lib/handlers/element.js
+++ b/lib/handlers/element.js
@@ -65,7 +65,7 @@ export function element(node, state) {
         continue
       }
 
-      prop = info.space
+      prop = info.space && prop !== 'className'
         ? hastToReact[info.property] || info.property
         : info.attribute
 
@@ -101,7 +101,13 @@ export function element(node, state) {
           }
         }
 
-        attributeValue = {
+        attributeValue = typeof value === 'string' ? {
+          type: "Literal",
+          start: 11,
+          end: 19,
+          value,
+          raw: `\"${value}\"`
+        } : {
           type: 'JSXExpressionContainer',
           expression: {type: 'ObjectExpression', properties: cssProperties}
         }

@DaniGuardiola
Copy link
Contributor Author

Wanna know something funny? This is the before and after of what I was trying to fix in my blog for the past 10 hours.

Before:

Screenshot from 2023-02-05 07-01-14

After (fixed):

Screenshot from 2023-02-05 07-13-40

I didn't even realize until I took this screenshot that the style was generally wonky in my formulas (spacing issues, etc). What bugged me was that missing fraction line.

I've spent my last 10 hours fixing a 10x0.5px black line in a LaTeX formula in my blog. No regrets though lmao. I've learned a lot about remark, rehype, hast, unified, etc. Time well spent! :)

...plus I can finally do fractions 😵‍💫

@DaniGuardiola
Copy link
Contributor Author

PD: I don't know much about AST. Do I need to add all of those properties (e.g. start, end, raw...)? I guess not all of them. Which ones are required?

@wooorm
Copy link
Member

wooorm commented Feb 5, 2023

Did you read the other discussion? This has all been explained already, and the solution is there! If you want to learn things than you're doing great though :)

wooorm added a commit that referenced this issue Feb 6, 2023
Previously, all properties from elements were passed using React casing.
This does not work, particularly for complex cases such as SVG and XML,
in any other JSX runtime.
Instead, all other JSX runtimes converge on allowing HTML casing.

This adds an option for that.

Related-to: #6.
Related-to: #7.
@wooorm wooorm closed this as completed in 7867d68 Feb 6, 2023
@wooorm wooorm added the 💪 phase/solved Post is done label Feb 6, 2023
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Feb 6, 2023
@DaniGuardiola
Copy link
Contributor Author

DaniGuardiola commented Feb 8, 2023

Hi @wooorm, sorry, I couldn't get back to this until now. I read the PR discussion some more, and checked the latest commits. This is great, thanks!

The last step for Solid Start (and other frameworks) users to be able to apply these settings would be to update @mdx-js/mdx so that the options can get to where this util is being called: https://github.com/mdx-js/mdx/blob/f4d78be5c015e8fab48589efae9507d0304e5e94/packages/mdx/lib/plugin/rehype-recma.js#L15

Shall I create an issue there?

@wooorm
Copy link
Member

wooorm commented Feb 8, 2023

Yup :) Feel free to open a feature request

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

No branches or pull requests

2 participants