-
-
Notifications
You must be signed in to change notification settings - Fork 2
Add preserveHtmlProp
option
#6
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
Conversation
Hi there! a) you‘re correct that this project turns hast property casing into React property casing |
It’s a bit different: when an author writes JSX like that, they’re writing JavaScript and choosing the casing. And we (in all of MDX and surrounding projects) don’t touch that. Note that JSX is also about components: it could just as well be With plugins, programmers have to use a specific casing for properties that relate to known HTML elements in plugins. That casing is different from React, which we generate here, but also different from HTML, which we generate in other places. So we must convert hast casing to something else. |
Thanks for the quick reply! Re (b), I'm not sure if "namespaces don't work in JSX" is part of the spec, reading the babel docs, it's allowed, but only React disallow it. So I think it's fair that Solid works that way. Re (c), I'm not sure of a clearer name 😅 Making it a react boolean prop doesn't feel scalable too. Re (d), the preact docs mentioned that you have to use Re (e), I can refactor to correct the function intent 👍 For Re (f), I can check that too after this is resolved. Re the comment above (#6 (comment)), I think I get the intent here. Here we're doing hast to esast, and how the generated JSX gets converted to HTML depends on the jsx runtime. I think the goal of this PR is to support more runtimes beside React's. |
Actually, since Preact has issues with props with colons, I can revert (e) for now so it's spreaded. That should be fine too for all runtimes. |
b) yes it’s allowed, which is why it’s allowed here — but by default it doesn’t work in babel, so things are going to crash, so solid should probably not require them I think the discussion mostly resolving around c) is the big one to discuss. 1) will this actually solve every other runtime? 2) will this solve one specific runtime? |
Two more things that you’ll likely run into:
|
Re (b), Solid has their own babel plugin so I think it's fine. I don't think it's fair to have all jsx runtimes rely on something like Re (c), that's a good point. I do (initially) want to output HTML attributes as I assume it's "vanilla JSX", and preserving
With the current PR (and 3rd commit), it does work for all runtimes. It would solve Preact/Solid/Astro's runtimes. The Maybe to scope things down, we introduce a |
If Solid has a custom Babel plugin, nothing we do here is ever going to fix Solid. We’d need to port that. And I’m not interested in porting that. That being said, Solid also seems to want to support actual JSX, with normal Babel plugin / esbuild / SWC. Although it’s having problems with actual JSX. See for example solidjs/solid#1483.
Nor is it fair for this project to include lists for every possible framework. That’s not feasible. The only way around that is for all these frameworks to converge on styles. Either React. Or indeed exact HTML attributes. But I don’t think these frameworks support exact HTML attributes.
How did you test?
Right indeed, something like that is what I am thinking. But I’d first like to verify that these frameworks actually support this. |
So I think we have a conclusion that we could generate JSX that's probably good enough to work in most JSX runtime. Anything unique would be behind an option. That way we don't have to explicitly support one runtime.
Since this already works with React, I'm testing with the hypothetical output of If you're asking about |
I’m not quite there yet. You’ve tested only It might be fine! But I want a good solution that’s verified to work, not something that might actually not work because no runtime implements it exactly. Some things I’m wondering about:
Some notes on Preact:
|
For a name, I think there are two options:
I don’t think we should use the word in the option, because this thing results in some things like “props” and some things like “attributes”. For |
Thanks for the summary and going through with me on this today 💚 I'll make some extra tests tomorrow to confirm what will and will not work so we get a stable feature out.
You need |
And cool :) thanks for your continued work! |
Made a few tests in Stackblitz for React, Preact, and Solid. Astro is still harder to test as Astro's JSX is only used in MDX, but you can imagine it as always rendering properties as-is and let's HTML handle the markup from there on. The result from the tests are that:
So I guess no3 (this PR) with spreading attr, would help Preact, Solid, and Astro, since they work fine with normal attribute names. Solid and Astro would work if hast contains We don't have full support for Solid yet due to I think I'm leaning towards (EDIT: I updated the Preact example. |
Thanks for your testing! I too spend the whole yesterday and until now today checking how all these frameworks work in weird edge cases, over at I put some examples in https://github.com/syntax-tree/hast-util-to-jsx-runtime/tree/main/example, which you could check out, to verify how all of this stuff works. I’d also welcome Astro as an example, it has a runtime I see, now we just need a simple way to turn those nodes into a DOM tree (perhaps you know of one?). I haven’t figured out a good way to cause Solid/Svelte/Vue to rerender and diff things. If you know how to cause that, that would be useful, as diffing might be a more complex case where this all breaks.
Svelte is completely broken on CSS or SVG. We could raise some of that stuff here: https://github.com/kenoxa/svelte-jsx. But it’s not maintained. So let’s skip that for now. Controlled inputs are broken for the dynamic examples. We can look at how to fix that some other time. One fun thing is that no framework other than React supports Luckily all frameworks other than React (ignoring Svelte) support HTML casing for attributes.
I totes agree with your “If you use Solid, turn this and this option on” example. Though, I did go with longer, more verbose names (
|
Wow thanks for the testing and the quick changes over at I tried to add Astro as an example (using this internal API), but it doesn't really work as it relies on Also not sure of a way to rerender in Solid/Svelte/Vue, usually they render once and then update internally by itself through state changes. Appreciate that you test Svelte JSX too, although I've rarely seen that used in the wild. I agree with the rest of your points, and the naming too. I prefer My time would be a bit tight so feel free to port the changes here to if you'd like. In that case I'm happy to close this PR too, or otherwise I can get to it early next week. Thanks again for looking into this! |
Initial checklist
Description of changes
Add
preserveHtmlProp
option to control the style of prop name produced:preserveHtmlProp: false
(default):preserveHtmlProp: true
:Some JSX runtimes, like Preact, Solid, and Astro don't support React's attr casing convention, so they have to be left as-is for it to render correctly.
I found this issue through MDX for Astro, while you can type
<x xlink:href="#test" />
as-is in a.mdx
file andremark-mdx
escapes it properly to render the attr as-is. If a rehype plugin injects hast, MDX doesn't escape it anymore (rehype comes after remark), making some rehype plugins not work in MDX + Preact/Solid/Astro.In this case, this blocks Astro: withastro/astro#5796
Note
Ideally, if this PR is merged, we have to expose another option from MDX so we can properly toggle in in Astro, as MDX uses it internally.
Since the repo doesn't have a lockfile,
npm install
raise errors which I fixed in the second commit. If this unrelated change is undesired, I can split and make a second PR.