Skip to content

Add JSX support in type definition #3

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

Merged
merged 11 commits into from
Dec 9, 2020
Merged

Add JSX support in type definition #3

merged 11 commits into from
Dec 9, 2020

Conversation

remcohaszing
Copy link
Member

This adds TypeScript support to use xastscript as JSX. This is just an idea. It’s definitely open for discussion. :)

The interface already supports JSX legacy mode, meaning this was already supported, except for types.

I don’t know if this was intended. Basically any function that accepts a tag name as first argument, props as second argument, and children as spread arguments, can be considered a JSX compatible function. I figured for xastscript it makes sense, because it’s for working with XML.

Some quirks:

  • JSX elements starting with upper case will be converted to variables, so XML tags starting with lower case can be used directly.
  • TypeScript doesn’t support namespaces in JSX.
  • Users may request support for the new JSX mode at some point.

@wooorm
Copy link
Member

wooorm commented Oct 31, 2020

Huh, interesting!

The unist-builder/hastscript/xastscript signatures are indeed designed to be somewhat like React.createElement, although they are all based on the earlier hyperscript.

The new JSX transform in React 17 is in my opinion nice for React, but unfortunate for anyone else who handles JSX. For example, it’s React-specific that children is a prop (and because they treat it as one, we can never have an actual children prop in xast). Another problem, which already exists, is that JSX is coupled to the framework: while both React and Vue support JSX, it’s not possible to write (more complex) JSX that works in both. Anyway, all different things.


Code-wise, some of the comment are still missing, or read a bit confusing to me.
This would probably also be useful to describe with an example in the readme?

There should be tests for members in tag names: <a.b />, and for attribute expressions: <a {...b} />.

Should we somehow handle fragments too? Turn into roots? Only makes sense when it‘s the outer thing, and should be extracted when occurring as a child, and thrown on when having props? 🤔

JSX allows tags/fragments as attribute values too: <welcome name=<>Venus</> />, <welcome name=<span>Pluto</span> />, which doesn’t make sense for us.

@wooorm
Copy link
Member

wooorm commented Oct 31, 2020

If we’re going to do this, it’d be wise to have actual checks for it in the tests too. But that could be a different PR. And we’d check acorn and others to see how they handle JSX.

@remcohaszing
Copy link
Member Author

Huh, interesting!

My thought exactly! :D

The unist-builder/hastscript/xastscript signatures are indeed designed to be somewhat like React.createElement, although they are all based on the earlier hyperscript.

Only one JSX pragma can be used per file. I think this might be interesting for hastscript as well, but not so much for unist-builder.

The new JSX transform in React 17 is in my opinion nice for React, but unfortunate for anyone else who handles JSX. For example, it’s React-specific that children is a prop (and because they treat it as one, we can never have an actual children prop in xast). Another problem, which already exists, is that JSX is coupled to the framework: while both React and Vue support JSX, it’s not possible to write (more complex) JSX that works in both. Anyway, all different things.

I agree. Let’s forget about this. :)

Code-wise, some of the comment are still missing, or read a bit confusing to me.

Type definitions for JSX are super confusing. Also JSX type definitions don’t line up with how JSX can be used in React or Preact.

This would probably also be useful to describe with an example in the readme?

I wanted to check if you think this is a good idea before continueing this. 😃

There should be tests for members in tag names: <a.b />, and for attribute expressions: <a {...b} />.

<a.b /> uses a functional component. This is the same situation as for <Bar />.

There is a test for props (<urlset xmlns={xmlns} />). I should probably add a test to show only string attributes are allowed. This is defined by IntrinsicElements.

Should we somehow handle fragments too? Turn into roots? Only makes sense when it‘s the outer thing, and should be extracted when occurring as a child, and thrown on when having props?

See below.

JSX allows tags/fragments as attribute values too: <welcome name=<>Venus</> />, <welcome name=<span>Pluto</span> />, which doesn’t make sense for us.

This is disallowed by the IntrinsicElements type.

If we’re going to do this, it’d be wise to have actual checks for it in the tests too. But that could be a different PR.

This would mean the tests need to be processed by Babel or TypeScript. This doesn’t mean it can’t be done.

And we’d check acorn and others to see how they handle JSX.

Acorn is a JavaScript parser. I don’t see why we should look how they handle JSX. xastscript implements the interface, like react, preact, or mini-jsx (which I wrote, including types).


I’ll try to give a brief explanation of what JSX is.

The following 2 blocks of code are identical:

/** @jsx x */
import * as x from 'hastscript';

function Component() {}

x('foo', null);
x('foo', { bar: 'baz' });
x('foo', null, x('bar', null), x('baz', null))
x(Component, null);

<foo />
<foo bar="baz" />
<foo>
  <bar />
  <baz />
</foo>
<Component />

JSX tag names matching [a-z_][\da-z_]* will be converted to string. All other components will be converted to identifiers. Props are simply objects, or null if no props are defined.

In TypeScript, the JSX.Element type defines the value of the returned JSX node. This is a bug. The return value could be dynamic on the implementation of the JSX function. This isn’t relevant for xastscript though.


A functional component is a library specific implementation detail. The actual call signature depends on the implementing library.

In React, a functional component is a function that has the following signature:

type ReactNode = string | number | boolean | null | JSX.Element | Array<ReactNode>;
function MyComponent(props?: ObjectLikeInterface): ReactNode;

In TypeScript, a functional component is a function that has the following signature:

function MyComponent(props?: ObjectLikeInterface): JSX.Element;

This is a bug in TypeScript. The return type of functional components should match the same type as the return value of the JSX implementation, which isn’t true for React and other libraries.


A fragment is a functional component that takes no props (or optional props only), whose name is marked as fragment. This means adding support for fragments actually means adding support for functional components.

/** @jsxFrag Foo */

// This is a fragment now.
function Foo() {
  return <div />
}

// yields <div></div>
<></>

Typically a fragment is used to return wrapped children.

For a fragment implementation xastscript the return type would be Array<xast.Node>, which is not a valid according to TypeScript, as it doesn’t match xast.Element.


Now the explanations of the added type definitions:

Element: The return type of a JSX expression, as well as the return type of functional components.

IntrinsicAttributes: Shared props allowed by all components. I.e. this is used to define key in React. This should be set to xast.Attributes, but setting this to never combined with how IntrinsicElements is used, disallows functional and class components while keeping support for string tag names.

ElementChildrenAttribute: This interface contains one key. This key defines as what prop children is passed to functional components. This also determines which key in IntrinsicElements defines the type of Children.

IntrinsicElements: is a mapping of known tag name types. Typically this is used to define which HTML or SVG elements support which props. In this case, any tag name may use xast.Attributes. The '' key matches the key in ElementChildrenAttribute, meaning it’s used to define the type of JSX children.

wooorm added a commit that referenced this pull request Nov 2, 2020
This PR tests that `xastscript` can be used as the pragma for JSX with bublé
and babel.

Code-wise, this adds support for using `x` to generate root nodes.
This is done by omitting the tag name (like so: `x()`, `x(null, 'child')`).
Previously, omitting a `name` resulted in an exception.

Another aspect of supporting JSX is supporting fragments as children.
As fragments yield root nodes, we unravel them and use only their children.
While this could be seen a change, xast prohibits roots occurring in nodes,
so the unraveling instead fixes what would otherwise be a broken tree.

Related to: GH-3.
@wooorm
Copy link
Member

wooorm commented Nov 2, 2020

If we’re going to do this, it’d be wise to have actual checks for it in the tests too. But that could be a different PR.

This would mean the tests need to be processed by Babel or TypeScript. This doesn’t mean it can’t be done.

See GH-4!

@ChristianMurphy ChristianMurphy added ☂️ area/types This affects typings 🧒 semver/minor This is backwards-compatible change labels Nov 2, 2020
wooorm added a commit that referenced this pull request Nov 3, 2020
This PR tests that `xastscript` can be used as the pragma for JSX with bublé
and babel.

Code-wise, this adds support for using `x` to generate root nodes.
This is done by omitting the tag name (like so: `x()`, `x(null, 'child')`).
Previously, omitting a `name` resulted in an exception.

Another aspect of supporting JSX is supporting fragments as children.
As fragments yield root nodes, we unravel them and use only their children.
While this could be seen a change, xast prohibits roots occurring in nodes,
so the unraveling instead fixes what would otherwise be a broken tree.

Related to: GH-3.
wooorm added a commit that referenced this pull request Nov 3, 2020
This PR tests that `xastscript` can be used as the pragma for JSX with bublé
and babel.

Code-wise, this adds support for using `x` to generate root nodes.
This is done by omitting the tag name (like so: `x()`, `x(null, 'child')`).
Previously, omitting a `name` resulted in an exception.

Another aspect of supporting JSX is supporting fragments as children.
As fragments yield root nodes, we unravel them and use only their children.
While this could be seen a change, xast prohibits roots occurring in nodes,
so the unraveling instead fixes what would otherwise be a broken tree.

Related to: GH-3.

Reviewed-by: Christian Murphy <[email protected]>
wooorm added a commit to syntax-tree/hastscript that referenced this pull request Nov 6, 2020
This change tests that `hastscript` can be used as the pragma for JSX with bublé
and babel.

Code-wise, this adds support for using `h` to generate root nodes.
This is done by omitting the tag name (like so: `h()`, `h(null, 'child')`).
Previously, omitting a `name` resulted in the default element to be created
(`div` for `h`, `g` for `s`).

This change thus is a breaking change.
The old behavior is still available when passing an empty string: `h('')`.

Another aspect of supporting JSX is supporting fragments as children.
As fragments yield root nodes, we unravel them and use only their children.
While this could be seen a change, hast prohibits roots occurring in nodes,
so the unraveling instead fixes what would otherwise be a broken tree.

Related to: syntax-tree/xastscript#3.
Related to: syntax-tree/xastscript#4.
@ChristianMurphy ChristianMurphy added 👶 semver/patch This is a backwards-compatible fix and removed 🧒 semver/minor This is backwards-compatible change labels Nov 6, 2020
@ChristianMurphy
Copy link
Member

switching to semver patch, since this was added as a feature in #4, this now catches the type definitions up to the implementation

@wooorm
Copy link
Member

wooorm commented Nov 11, 2020

...soooo, which one should we go with? Both are failing 🤷‍♂️ Anything unresolved things? Other ways I can help?

This means the type of `hastscript.JSX.Element` had to be changed to
`xast.Element | xast.Root`. This also means a minimum TypeScript version of 4.0
is required to test types.
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @remcohaszing!

It’s value is not supposed to be used.
@remcohaszing
Copy link
Member Author

Ping @wooorm

I think this can be merged and released 😄

@wooorm wooorm merged commit da82f9f into syntax-tree:main Dec 9, 2020
@wooorm
Copy link
Member

wooorm commented Dec 9, 2020

thanks, released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix
Development

Successfully merging this pull request may close these issues.

3 participants