✨Add support for functional elements #19

cowboyd · 2023-06-23T10:12:20Z

Description of changes

resolves #17

The nice thing about JSX is that it is "just javascript" which means that you can add abstractions to it with simple functions. However, the current implementation of JSX in hastscript stops short of allowing elements to be defined this way.

This adds this support so you can make dead simple HTML templates in JavaScript: functions that accept parameters and return a JSX.Element

TODOs and Open Questions

figure out what to do with the "classic" JSX transform since it is currently just a call through to the native h() function. We either need to wrap this function or add support for function expansion inside it as well.
Sort out typings, especially for JSX.ElementChildrenAttribute

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

cowboyd · 2023-06-23T12:03:01Z

Looks like I got the capitalization of the "description of changes" wrong. Also, might be out of order (I moved it up to the top)

It looks like the code linter is failing, but hard to figure out where.

lib/runtime.js

test-d/automatic-s.tsx

test/jsx.jsx

wooorm · 2023-06-23T15:57:40Z

Looks like I got the capitalization of the "description of changes" wrong. Also, might be out of order (I moved it up to the top)

You added undone todos, which is why it says it’s not ready yet!

You can run npm test locally to do everything CI does here.
The crash in CI is because of a XO bug: if you update to 54 here:

hastscript/package.json

Line 81 in e031133

"xo": "^0.53.0"

, it works again! Sorry about that

cowboyd · 2023-06-27T17:39:20Z

@wooorm Do you have any thoughts on how to implement the "classic" runtime? As far as I can tell it is a pass through to the h function which would mean need to either wrap it, or add the ability to have functional components to h as well.

wooorm · 2023-06-28T09:11:54Z

Depends on whether we want to support h(Component) directly here or not.
Easiest is probably a new different wrapper function first?

remcohaszing · 2023-06-29T14:32:41Z

jsx-factory.js

+ */
+export function h(typeOrFn, props, ...children) {
+  if (typeof typeOrFn === 'function') {
+    return typeOrFn({...props, children})


I think it would be nice to infinitely flatten children, so we can pass this in JSX:

function Foo({ children }) { // children is ['bar', 'baz', 'fooz'] } <Foo> {'bar'} {['baz']} {[[[['fooz']]]]} <Foo>

This is just a silly hardcoded example, but in practice I think it’s common to use a lot of .map() and .filter() with JSX.

@remcohaszing Do you think this should be a blocker?

Yes, because changing it later would be a breaking change.

There are cases where children is currently not an array, right? (i saw a test for a function). Do we even want to support that?

if anything can be passed (such as functions), that also means a user can pass an array by reference, which we shouldn’t touch, how to handle that?

We can just create a flat copy. It should be as simple as:

return typeOrFn({...props, children: [children].flat()})

And we need to do something similar for the automatic runtime.

We could go a step further and also flatten root nodes. That requires a bit more additional code.

I think children is just an iterable of values that have meaning to the function. If that function creates an intrinsic element like A, then it makes no sense to have children that are anything other than child nodes. However, if it is a function component, then it's up to the function how it wants to interpret its arguments.

I would say you definitely do not want to flatten children in the case of a component, but it would be an option for creating low level html nodes. I don't have enough experience with what flattening would feel like to have an opinion.

The thing is, in this example, Component gets a children prop as a string.

<Component> {'foo'} </Component>

In this example, it’s an array of two strings.

<Component> {'foo'} {'bar'} </Component>

React normalizes this. I think it’s a good idea to do this here as well. It makes sense to me to always pass a flat array, but I’m open to other suggestions as well. Perhaps strings should even be transformed to text nodes?

React normalizes this.

That’s not what React does, it’s how Babel and others do their JSX compilation. And there’s as way around it: by passing children. children is just a prop. <X children={SomeFunctionOrClassOrWhatever} /> or so is fine.

Some examples: https://babeljs.io/repl#?browsers=defaults%2C%20not%20dead&build=&builtIns=false&corejs=3.21&spec=false&loose=false&code_lz=DwQQBAxgFglgNgEwE4FMB2BeA3gbQEQCGeANGAIwC6AvgHy54BGJYATNfRMwMykQD2aAM584KAHRw-Ac2rAA9CBpA&debug=false&forceAllTransforms=false&modules=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env%2Creact%2Cstage-2&prettier=false&targets=&version=7.22.5&externalPlugins=&assumptions=%7B%7D

I don’t think we should touch children. a) React and similar don’t either, b) it would be slow to iterate / call .flat() on every children array?

wooorm

Nice work! Some initial nitpicks on the code!

jsx-factory.js

wooorm · 2023-06-30T11:44:41Z

jsx-factory.js

+ * @typedef {(props: JSXProps) => HResult } HFn
+ */
+
+import {h as hast} from './index.js'


This doesn’t support SVG

Instead of creating another factory, and an HTML and SVG function (and in the future mathml), I think I now prefer to just add this to the regular h; also the affect on the docs gets a bit complex with a separate /jsx-factory?

Ok, so to confirm, we'll enhance the core h() function to accept null, string, or function. Do you want me to document this feature? It seems like it's not very useful when you are using hypersrcipt because you can just call the function yourself. I.e. why say:

h(DLEntry, { key: "x", value: "y" })

When you could just as easily say:

DLEntry({ key: "x", value: "y" });

There might be a reason to invoke it with the former, but I can't think of it. What if I just note on the @param declaration "accepts a function in order to support JSX"

React and preact are the same? React.createElement(DlEntry, {key, value}) vs DlEntry({key, value}) vs <DlEntry key={key} value={value})>. The main reason is that you don’t have to create 2 wrappers and explain to users that one h is different from another h

I agree that this is simpler. I'm just asking how to explain to users who don't use JSX why this is also something that the h function can do:

h(DLEntry, { key: "x", value: "y" })

If I wasn't using JSX, I would be like "why is that ever useful", so I was suggesting a brief note in the jsdoc that this is for JSX support.

jsx-factory.js

lib/jsx-automatic.d.ts

lib/runtime.js

test-d/automatic-h.tsx

test/jsx.jsx

wooorm · 2023-06-30T11:54:07Z

test/jsx.jsx

+      ])
+    },
+    {depth: 20}
+  )


Console call should be removed when done

cowboyd · 2023-07-01T15:23:42Z

Here's an attempt at what we discussed. It simplifies things greatly to have everything flowing through a single codepath. However, it looks like the coverage tests are failing?

Unfortunately, I'm nearing the end of the amount of time I can spend on this and there is still the coverage to sort out and also the issue of to-flatten or not-to-flatten remaining.

wooorm · 2023-07-03T07:13:59Z

lib/core.js

@@ -58,18 +62,38 @@ export function core(schema, defaultTagName, caseSensitive) {
     *   (selector: null | undefined, ...children: Array<HChild>): Root
     *   (selector: string, properties: HProperties, ...children: Array<HChild>): Element
     *   (selector: string, ...children: Array<HChild>): Element
+     *   (component: HComponent, props: HComponentProperties, ...children: Array<HChild>): HResult


should we support h(Component)? h(Component, null)? h(Component, Child)

And h(Component, {children: ['a']}, 'b')?

wooorm · 2023-07-03T07:18:42Z

lib/core.js

@@ -98,6 +122,7 @@ export function core(schema, defaultTagName, caseSensitive) {
              }
            }
          } else {
+            // @ts-expect-error cannot be `HComponentProperties` because we're not on that codepath.


Wouldn’t it be easier to reuse HProperties for props, instead of a different but similar HComponentProperties?
That would solve this. And probably make it easier to create a component MyLink which wraps a and internally calls h('a', {...props, something: true}), which would currently fail I think?

wooorm · 2023-07-03T07:34:00Z

jsx-factory.js

+ */
+export function h(typeOrFn, props, ...children) {
+  if (typeof typeOrFn === 'function') {
+    return typeOrFn({...props, children})


React normalizes this.

That’s not what React does, it’s how Babel and others do their JSX compilation. And there’s as way around it: by passing children. children is just a prop. <X children={SomeFunctionOrClassOrWhatever} /> or so is fine.

Some examples: https://babeljs.io/repl#?browsers=defaults%2C%20not%20dead&build=&builtIns=false&corejs=3.21&spec=false&loose=false&code_lz=DwQQBAxgFglgNgEwE4FMB2BeA3gbQEQCGeANGAIwC6AvgHy54BGJYATNfRMwMykQD2aAM584KAHRw-Ac2rAA9CBpA&debug=false&forceAllTransforms=false&modules=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env%2Creact%2Cstage-2&prettier=false&targets=&version=7.22.5&externalPlugins=&assumptions=%7B%7D

I don’t think we should touch children. a) React and similar don’t either, b) it would be slow to iterate / call .flat() on every children array?

cowboyd · 2023-07-03T08:06:07Z

I'm going to park this for now because it seems like there are still several loose ends that need to be sorted out, and I'm officially over my time budget for this. I ended up creating a simple adapter directly from JSX to HAST that satisfies my immediate need, and that I published here: https://github.com/thefrontside/hastx

I'd love to see somebody else perhaps use this as a blueprint to push this forward (or not), but either way, it was a pleasure to have made the attempt.

github-actions bot added the 👋 phase/new Post is being triaged automatically label Jun 23, 2023

This comment has been minimized.

Sign in to view

cowboyd force-pushed the functional-jsx-elements branch from 21504df to 663455e Compare June 23, 2023 10:13

cowboyd mentioned this pull request Jun 23, 2023

Allow functional components in JSX #17

Closed

4 tasks

cowboyd force-pushed the functional-jsx-elements branch from 663455e to af30cad Compare June 23, 2023 10:47

wooorm reviewed Jun 23, 2023

View reviewed changes

lib/runtime.js Outdated Show resolved Hide resolved

test-d/automatic-s.tsx Show resolved Hide resolved

test/jsx.jsx Show resolved Hide resolved

wooorm requested a review from remcohaszing June 23, 2023 15:57

cowboyd force-pushed the functional-jsx-elements branch 4 times, most recently from c20acd7 to c1e22bd Compare June 29, 2023 12:57

cowboyd marked this pull request as ready for review June 29, 2023 13:07

cowboyd requested a review from wooorm June 29, 2023 13:07

remcohaszing reviewed Jun 29, 2023

View reviewed changes

cowboyd force-pushed the functional-jsx-elements branch 3 times, most recently from e87af51 to e6f69d9 Compare June 29, 2023 19:42

cowboyd requested a review from remcohaszing June 29, 2023 20:37

wooorm reviewed Jun 30, 2023

View reviewed changes

cowboyd force-pushed the functional-jsx-elements branch 2 times, most recently from 47e8d4f to f1b4d19 Compare July 1, 2023 15:17

✨Add support for function elements

890a9d4

cowboyd force-pushed the functional-jsx-elements branch from f1b4d19 to 890a9d4 Compare July 1, 2023 15:19

cowboyd requested a review from wooorm July 1, 2023 15:20

wooorm reviewed Jul 3, 2023

View reviewed changes

cowboyd closed this Jul 3, 2023

This comment has been minimized.

Sign in to view

wooorm added the 🤷 no/invalid This cannot be acted upon label Jul 3, 2023

github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 👋 phase/new Post is being triaged automatically labels Jul 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Add support for functional elements #19

✨Add support for functional elements #19

cowboyd commented Jun 23, 2023 •

edited

Loading

This comment has been minimized.

cowboyd commented Jun 23, 2023

wooorm commented Jun 23, 2023

cowboyd commented Jun 27, 2023

wooorm commented Jun 28, 2023

remcohaszing Jun 29, 2023

cowboyd Jun 29, 2023

remcohaszing Jun 30, 2023

wooorm Jun 30, 2023

remcohaszing Jun 30, 2023

cowboyd Jun 30, 2023

remcohaszing Jun 30, 2023

wooorm Jul 3, 2023

wooorm left a comment

wooorm Jun 30, 2023

cowboyd Jun 30, 2023

wooorm Jun 30, 2023 •

edited

Loading

cowboyd Jun 30, 2023

wooorm Jun 30, 2023

cowboyd commented Jul 1, 2023

wooorm Jul 3, 2023

wooorm Jul 3, 2023

wooorm Jul 3, 2023

wooorm Jul 3, 2023

cowboyd commented Jul 3, 2023

This comment has been minimized.

✨Add support for functional elements #19

✨Add support for functional elements #19

Conversation

cowboyd commented Jun 23, 2023 • edited Loading

Description of changes

Initial checklist

This comment has been minimized.

cowboyd commented Jun 23, 2023

wooorm commented Jun 23, 2023

cowboyd commented Jun 27, 2023

wooorm commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooorm Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowboyd commented Jul 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowboyd commented Jul 3, 2023

This comment has been minimized.

cowboyd commented Jun 23, 2023 •

edited

Loading

wooorm Jun 30, 2023 •

edited

Loading