Skip to content

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

Closed
wants to merge 4 commits into from
Closed

Add preserveHtmlProp option #6

wants to merge 4 commits into from

Conversation

bluwy
Copy link

@bluwy bluwy commented Feb 1, 2023

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Add preserveHtmlProp option to control the style of prop name produced:

preserveHtmlProp: false (default):

<>
  <x xlinkHref="#test" />
</>

preserveHtmlProp: true:

<>
  <x xlink:href="#test" />
</>

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 and remark-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.

@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 1, 2023
@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

Hi there!

a) you‘re correct that this project turns hast property casing into React property casing
b) those red squiggles are very important, “namespaces” don’t work in JSX normally. They’re fine in MDX because I see no reason in crashing on them, but I think that Solid/etc will have to support fields without colons if they say they support JSX.
c) There are practically likely differences between Astro, Solid, Svelte, Preact, etc. I don’t think you can group them under the name “HTML”
d) I believe Preact supports those React casing conventions as well, at least, they want you to type them as such: https://github.com/preactjs/preact/blob/9d4b2dc0e1ea9b5855f751049d1563a351da9102/src/jsx.d.ts#L348-L359, so perhaps this is just Solid/Astro problems.
e) The jsxIdentifierName(prop, state.jsxStyle === 'html') stuff is incorrect. That function is for checking JSX identifiers, and a:b is not a single JSX identifier. It’s two identifiers in a namespace. I’m fine with supporting colons in props! But, this isn’t the way (it’s also buggy, e.g., allowing a:, which is incorrect).
f) the same problem exists in hast-util-to-jsx-runtime

@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

I found this issue through MDX for Astro, while you can type <x xlink:href="#test" /> as-is in a .mdx file and remark-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.

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 <MyComponent MyFunky:Prop />

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.

@bluwy
Copy link
Author

bluwy commented Feb 1, 2023

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. html seems to be closest to the "style" of JSX output. Maybe 'react' | 'jsx' as react enforces a subset of the JSX spec for prop casing?

Re (d), the preact docs mentioned that you have to use preact/compat to get camelCase working. Not sure why the types are there other than preactjs/preact#1870 (comment). In part, I think you're right too (stackblitz example) but it looks like a Preact tooling issue. The example doesn't render the attributes right when using camelCase too (but I bet preact/compat would fix that)

Re (e), I can refactor to correct the function intent 👍 For a: namespace, I guess we can relax and allow for now as that's what's given by the hast?

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.

@bluwy
Copy link
Author

bluwy commented Feb 1, 2023

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.

@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

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
c) JSX (the grammar) does not dictate casing, so I don’t think that’s a good name. Which name to pick depends on your goal: 'html' could be fine, if you really want to output HTML attributes (e.g., aria-label), however, I don’t think you do, because my assumption here is that Preact/Solid/Svelte/Astro/whatever are all going to have different things they expect. And then I’m not sure how useful this feature is. Or that name is.
e) as you later noted, no, this is intentional: an identifier (or a namespace), has to be a valid in JSX, any thing else can still be used, but it must be spread

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?

@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

Two more things that you’ll likely run into:

@bluwy
Copy link
Author

bluwy commented Feb 1, 2023

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 property-information so it's compatible with React's flavour of JSX. That increases the bundle size.

Re (c), that's a good point. I do (initially) want to output HTML attributes as I assume it's "vanilla JSX", and preserving style as a string should be part of it, but that won't work with some flavours of JSX.

  1. will this actually solve every other runtime? 2) will this solve one specific runtime?

With the current PR (and 3rd commit), it does work for all runtimes. It would solve Preact/Solid/Astro's runtimes. The style object works for them, though I know this can be false for future JSX runtimes, we can probably fix with the ecosystem in hand for now.

Maybe to scope things down, we introduce a preserveHtmlProp: boolean option instead? If a new runtime wants style as a string, that can be another option, so we don't have an option that's 'react' | 'solid' | 'balloonjs' in the future. Option name could be better perhaps, always hard.

@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

Solid has their own babel plugin so I think it's fine.

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.

I don't think it's fair to have all jsx runtimes rely on something like property-information so it's compatible with React's flavour of JSX. That increases the bundle size.

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.

it does work for all runtimes

How did you test?

preserveHtmlProp: boolean

Right indeed, something like that is what I am thinking. But I’d first like to verify that these frameworks actually support this.

@bluwy
Copy link
Author

bluwy commented Feb 1, 2023

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.

How did you test?

Since this already works with React, I'm testing with the hypothetical output of jsxStyle: 'html' (which option would be scoped down later), since that "unlocks" working with other runtimes. The solid example like before and this new preact stackblitz should show. Astro is harder to show but I've tested it locally which works for this PR.

If you're asking about style object working specifically, Preact's example above, Solid, and Astro should show it.

@bluwy bluwy changed the title Add jsxStyle option Add preserveHtmlProp option Feb 1, 2023
@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

So I think we have a conclusion that we could generate JSX that's probably good enough to work in most JSX runtime.

I’m not quite there yet. You’ve tested only xmlns:xlink and xlink:href. And only tested whether they are cased correctly in the final HTML. I don’t think that’s enough to state that using HTML casing for every attribute will work in most JSX runtimes.
I think we should:
1) test more properties
2) test whether they actually work: e.g., does the xlink:href actually work by linking an actual thing?

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:

  • What about event handlers for example? Setting onclick="console.log(a)" in Solid will show that it wants attr:onclick instead. Preact crashes. This might not matter here, because strings of JS are not likely to work in JSX things anyway, and aren’t a great idea for security anyway.
  • What about things on inputs, such as value and checked? Maybe it doesn’t matter in this project (as here we’re talking about generating JavaScript code which will run once, not function calls that depend on state and will result in a different DOM tree eventually). But I at least remember from React that it wants you to use properties for input value things and otherwise will warn.
  • On style: I just checked like <div style={{ backgroundColor: 'green' }}>qwe</div> in the Solid playground, which doesn’t work. So we’d need to pass that as a string.

Some notes on Preact:

@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

For a name, I think there are two options:

  • elementAttributeNameCase: 'html' | 'react' (or a shorter version such as attributeNameCase), where react is current and html is what you are proposing. This allows for future new schemes.
  • useHtmlAttributeNameCasing (or a shorter version such as useHtmlCasing). This is similar to your current preserveHtmlProp.

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”.
“Preserve” is also not correct. There is nothing to preserve. There is only to use one format or another.


For style, maybe passStyleObject: boolean? Or even styleAttributeValue: 'html' | 'react' (I don’t like neither of these names though)

@bluwy
Copy link
Author

bluwy commented Feb 1, 2023

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.

  • On style: I just checked like <div style={{ backgroundColor: 'green' }}>qwe</div> in the Solid playground, which doesn’t work. So we’d need to pass that as a string.

You need <div style={{ 'background-color': 'green' }}>qwe</div> for it to work, which is another problem since style-to-object camelCases it by default. Ah JSX tooling 🥲

@wooorm
Copy link
Member

wooorm commented Feb 1, 2023

style-to-object is specifically in use because React wants it. Normal CSS casing doesn’t work with React! So it isn’t about that library, it’s about what React expects.

And cool :) thanks for your continued work!

@bluwy
Copy link
Author

bluwy commented Feb 2, 2023

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:

  1. hast onclick doesn't really work when turned to JSX for React and Preact (needs function as value). Solid works.
  2. hast checked/value works with Preact and Solid. React needs defaultChecked/defaultValue to really work in an uncontrolled state.
  3. xlink: and xmlns: props needs to be camelCase in React and Preact (Preact has partial support like you linked above, as it doesn't work for xmlns:). Solid works without camelCase. Spreading the attr as-is works for all of them.

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 onclick.

We don't have full support for Solid yet due to style, which could be another option. We also don't have full support for React & Preact due to onclick needing to be a function in JSX, maybe it can turn to ={() => ...}, and another option to disable.


I think I'm leaning towards useHtmlCasing as the name if we go forward. That way the consumer dictates the JSX output, and there could be a "If you use Solid, turn this and this option on" flexibility. I'd be fine having the default feature set cater for React's style of JSX first.

(EDIT: I updated the Preact example. onclick={()=>{}} works.)

@wooorm
Copy link
Member

wooorm commented Feb 3, 2023

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 hast-util-to-jsx-runtime.

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.
And that diffing will likely break controlled inputs.

Thing React (dynamic) Preact (dynamic) Solid (static) Svelte (static) Vue (static)
event handler as string
controlled inputs
CSS style props DOM DOM CSS DOM
SVG attributes React HTML HTML HTML
xlink:href React HTML HTML HTML
xml:lang React

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.
I also understand why most frameworks don’t go and eval strings of JavaScript. I don’t think that’s for us to do either. So not supporting them is fine with me.

One fun thing is that no framework other than React supports xml:lang. As in, really supports it.
To support that you have to add the attribute with an XML namespace, and I guess nobody does.
It’s rather obsolete though, so let’s ignore it.

Luckily all frameworks other than React (ignoring Svelte) support HTML casing for attributes.
They all (including React) support DOM casing for style objects (except Solid).
I added support for that as follows:

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 (elementAttributeNameCase: 'html' | 'react, stylePropertyNameCase: 'css' | 'dom):

  • useHtmlCasing doesn’t say what it affects (element attribute names? style property names?)
  • the words attribute and property are both vague and used in lots of places, hence elementAttribute and styleProperty
  • the word style is also vague when we’re talking about the “style” of names to use
  • instead of 'react' I could’ve gone with 'dom' there too? But I though React was slightly different from actual DOM casing 🤔

@bluwy
Copy link
Author

bluwy commented Feb 3, 2023

Wow thanks for the testing and the quick changes over at hast-util-to-jsx-runtime! I cloned the repo down, and it's a lot better of a test that mine 😄

I tried to add Astro as an example (using this internal API), but it doesn't really work as it relies on this.result. withastro/roadmap#462 might help in the future.

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 'react' instead of 'dom' as well since I don't think there's DOM casings for html attributes, unlike CSS where there's el.style.backgroundColor.


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!

wooorm added a commit that referenced this pull request 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 in 7867d68 Feb 6, 2023
@github-actions

This comment has been minimized.

@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
@bluwy bluwy deleted the support-html-jsx branch February 7, 2023 10:25
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 this pull request may close these issues.

2 participants