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

Allow u(type, props, ...children) form of API #10

Closed
4 tasks done
Josh-Cena opened this issue May 18, 2022 · 10 comments
Closed
4 tasks done

Allow u(type, props, ...children) form of API #10

Josh-Cena opened this issue May 18, 2022 · 10 comments
Labels
👎 phase/no Post cannot or will not be acted on

Comments

@Josh-Cena
Copy link

Josh-Cena commented May 18, 2022

Initial checklist

Problem

I'm probably doing something cursed, but I'm trying to see if it's able to write unists in JSX. I reproduced the example in the README as:

import { u } from "unist-builder";

const tree = (
  <root>
    <subtree id={1} />
    <subtree id={2}>
      <node>
        <leaf>leaf 1</leaf>
        <leaf>leaf 2</leaf>
      </node>
      <leaf id={3}>leaf 3</leaf>
      <void id={4} />
    </subtree>
  </root>
);

And using u as the JSX factory. Unfortunately, this didn't work, because u only accepts an array of children. In order to make this work, I have to use a single array as children everywhere.

Solution

I propose to make this valid:

var tree = u(
  "root",
  null,
  u("subtree", { id: 1 }),
  u(
    "subtree",
    { id: 2 },
    u("node", null, u("leaf", null, "leaf 1"), u("leaf", null, "leaf 2")),
    u("leaf", { id: 3 }, "leaf 3"),
    u("void", { id: 4 })
  )
);

Because JSX always compiles to type, props, ...children, whenever there are more than three arguments, the rest must all be children, and we can simply collect them into an array.

Alternatives

Do not use JSX for unist? 😄

I'm really curious if I'm able to make it work, though

@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 May 18, 2022
@wooorm
Copy link
Member

wooorm commented May 18, 2022

Perhaps better to add support for the modern automatic runtime, like https://github.com/syntax-tree/hastscript#jsx and xastscript?

@Josh-Cena
Copy link
Author

Can we do both? Hastscript also supports both the h(selector?[, properties][, …children]) API and automatic runtime.

@wooorm
Copy link
Member

wooorm commented Oct 27, 2022

I’d prefer only an automatic JSX runtime. I didn’t really like it myself when they announced it, but with my MDX work, I now really really prefer it over the classic runtime.
I’m open to a classic runtime, but perhaps we can tell people to import something else that isn’t u from unist-builder?

Main important points for me:

  • I don’t want to do breaking changes to the API only if it’s “nice for a couple folks”
  • I don’t want to weigh regular users down with more code
  • I’m open to (experimenting with) an improved JSX experience

If those are met, I’m 👍

@remcohaszing
Copy link
Member

I don’t see any problem breaking changes when adding support to a classic runtime. Currently the API accepts a string or an array of objects as its third argument.

assert.deepEqual(
  <root>
    string
  </root>,
  { type: "root", value: "string" }
)

assert.deepEqual(
  <root>
    <child />
  </root>,
  { type: "root", children: [ { type: 'child' } ] }
)

assert.deepEqual(
  <root>
    <child />
    <child />
  </root>,
  { type: "root", children: [ { type: 'child' }, { type: 'child' } ] }
)

assert.throws(
  () => (
    <root>
      string
      <child />
    </root>
  ),
  'Expected either a string or an array of nodes, got invalid array'
)

@remcohaszing
Copy link
Member

Should fragments get a special meaning?

@wooorm
Copy link
Member

wooorm commented Oct 28, 2022

JSX in the classic runtime only accepts children (Array<*>). Where * is an actual node or the result value of an expression, which is typically turned into a text node.
JSX only passes children as variadic parameters: not an array of children. So we can’t know whether a node is supposed to be a parent, but doesn’t have children.

  • JSX will pass multiple strings. We don’t know what to do with strings (how to handle b in <>a{}b</>)
  • JSX will pass strings after the first. We don’t know what do do with non-first strings (how to handle b in <><x />b</>)
  • JSX will pass numbers, null, undefined, and other values from expressions through as children. We do not know how to handle those.
  • JSX will not pass anything for no children. The intention could be an empty parent or a void node (<x />)
  • We indeed do not know what a fragment is.

unist accepts anything. For JSX, everything is a parent, and text nodes are a certain thing. There’s a mismatch there that can’t be solved without domain knowledge. That‘s why hast/mdast/xast/etc can work well with JSX, but I don’t think this can.

@remcohaszing
Copy link
Member

  • JSX will pass multiple strings. We don’t know what to do with strings (how to handle b in <>a{}b</>)
  • JSX will pass strings after the first. We don’t know what do do with non-first strings (how to handle b in <><x />b</>)

You’re right, my bad. Still this seems fine?

u('root', null, 'value')

assert.deepEqual(
  u('root', null, u('child', null), u('child', null)),
  u('root', null, [u('child', null), u('child', null)])
)

assert.throws(() => {
  u('root', null, 'value', 'value?')
}, 'Unexpected arguments, one string value is allowed, got: "value?"')
  • JSX will pass numbers, null, undefined, and other values from expressions through as children. We do not know how to handle those.

I don’t see any problems here. One could also pass plain values when writing plain JavaScript. This can be checked both at runtime and using TypeScript.

JSX will not pass anything for no children. The intention could be an empty parent or a void node (<x />)

Isn’t this already supported? From the readme:

u('void', {id: 4})

unist accepts anything. For JSX, everything is a parent, and text nodes are a certain thing. There’s a mismatch there that can’t be solved without domain knowledge. That‘s why hast/mdast/xast/etc can work well with JSX, but I don’t think this can.

I think it could work. I definitely see the similarities between JSX and unist. Yes, JSX has text nodes, but that doesn’t mean should allow them. We could disallow them using TypeScript, even though a single string child would still technically work.

I’m not sure it should be supported though. I like that it’s supported by hastscript and xastscript, because JSX looks a lot like HTML and XML. For this package it seems a bit more like a coincidence.

@wooorm
Copy link
Member

wooorm commented Oct 28, 2022

Isn’t this already supported? From the readme:

It could be fine. But, according to unist there are three shapes of nodes:

  • void (it must not have value and children fields).
  • literal (it must have a value field)
  • parent (it must have a children field)

Users will try to pass <x />, <x>stuff</x>, and <x><y /></x>, and expect them to generate a certain node interface, but it will generate three different node interfaces, two of them then, according the the unist rules, will be invalid.
I think it’s too easy to generate invalid unist trees like this.
And I don‘t see a way around it.

Still this seems fine?

A root that has a value field and does not have a children field is not fine according to mdast/hast/xast. Throwing on multiple strings is possible though. But then still, you’re generating a root with a value field.

@wooorm
Copy link
Member

wooorm commented Jan 23, 2023

I think JSX is a bad idea at this abstraction (see previous comment).

As for accepting a node instead of the current array of nodes or the value, that would get complex if props are also omitted, because a node matches the props interface, however, we can be sure that a node has a type: string field, whereas that would not make sense in props (because an explicit type: string is passed as the first parameter):

u('element', {type: 'text', value: 'whatever'})

Accepting ...children is also ambiguous if children is empty. That signature indicates that a parent has to be build, but that can’t be inferred, instead a void will be made:

const children = []
u('element', {whatever: true}, ...children) // {type: 'element', whatever: true}
u('element', ...children) // {type: 'element'}

As it’s all so ambiguous, so many edge cases that an author needs to be aware of, I’m inclined to just not do it.

What do you think @Josh-Cena? Haven‘t heard from you in a while :)

@wooorm
Copy link
Member

wooorm commented Feb 6, 2023

Ok closing, I’m not really sure about the trade offs!

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Feb 6, 2023
@wooorm wooorm added the 👎 phase/no Post cannot or will not be acted on label Feb 6, 2023
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

3 participants