The $ macro and new JSX transform

rome-user commented 1 year ago

According to the new React documentation released today, React.createElement is a legacy API, and JSX itself has used a new transform that will allow React to perform certain optimizations in the future. The new transform is designed to be backwards compatible with existing code.

Is this something worth investigating? Possibly refactoring the $ macro to match the modern JSX output?

lilactown commented 1 year ago

yeah it's worth investigating. not high priority right now since createElement still exists, but the new jsx transform is slightly more performant which is nice.

rome-user commented 1 year ago

I've decided to take a quick look at how the jsx function works. In short, it is a function that accepts arguments (type props key), but there is a catch: there is also a "development version" of this function jsxDEV accepting arguments (type config maybeKey source self). Further complicating things, both jsx and jsxDEV exist in separate modules. The former exists in react/jsx-runtime and the latter exists in react/jsx-dev-runtime.

The intention of the jsxDEV function seems to be to make it easier for development tools to do things like verify props are not using reserved names and annotate ReactElement objects with filenames and line numbers.

Quick demonstration

(react/createElement "h1" nil "Hello world")
;; => {"$$typeof" "Symbol(react.element)", "type" "h1", "key" nil, "ref" nil, "props" #js {:children "Hello world"}, "_owner" nil, "_store" #js {}}

(jsx "h1" #js {:children "Hello world"})
;; => {"$$typeof" "Symbol(react.element)", "type" "h1", "key" nil, "ref" nil, "props" #js {:children "Hello world"}, "_owner" nil, "_store" #js {}}

Something that surprised me was the following:

(jsx "h1" #js {:children "Hello world"} nil)
;; => {"$$typeof" "Symbol(react.element)", "type" "h1", "key" "null", "ref" nil, "props" #js {:children "Hello world"}, "_owner" nil, "_store" #js {}}

This seems to imply nil is a valid React key, and the only to provide no key is by omitting the function argument.

rome-user commented 1 year ago

Here's a patch I made to attempt getting $ to emit calls to jsx function.

diff --git a/src/helix/core.cljs b/src/helix/core.cljs
index 83d1aff..17c4ae8 100644
--- a/src/helix/core.cljs
+++ b/src/helix/core.cljs
@@ -4,7 +4,8 @@
             [helix.impl.props :as impl.props]
             [helix.impl.classes :as helix.class]
             [cljs-bean.core :as bean]
-            ["react" :as react])
+            ["react" :as react]
+            ["react/jsx-runtime" :refer [jsx]])
   (:require-macros [helix.core]))

@@ -57,18 +58,26 @@
                     (:native (meta type)))
         type' (if (keyword? type)
                 (name type)
-                type)]
-    (if (map? ?p)
-      (apply create-element
-             type'
-             (if native?
-               (impl.props/-dom-props ?p)
-               (impl.props/-props ?p))
-             ?c)
-      (apply create-element
-             type'
-             nil
-             args))))
+                type)
+        children (clj->js (if (or (nil? ?p)
+                                  (map? ?p))
+                            ?c
+                            args))
+        props' (if (map? ?p)
+                 ;; NOTE: May want to dissoc :key since it is a dedicated
+                 ;; argument in the jsx function.
+                 (if native?
+                   (impl.props/-dom-props ?p)
+                   (impl.props/-props ?p))
+                 #js {})
+        ?key (.-key props')]
+    (when (seq children)
+      (set! (.-children props') (if (< 1 (count children))
+                                  children
+                                  (first children))))
+    (if ?key
+      (jsx type' props' ?key)
+      (jsx type' props'))))

Unfortunately, I am getting warnings about duplicate keys when I test out the following code

($ "div"
   ($ "p" "One")
   ($ "p" "Two"))

The output I get is an object like so

{"$$typeof" "Symbol(react.element)", "type" "div", "key" nil, "ref" nil, "props" #js {:children #js [#js {"$$typeof" "Symbol(react.element)", :type "p", :key nil, :ref nil, :props #js {:children "One"}, :_owner nil, :_store #js {}} #js {"$$typeof" "Symbol(react.element)", :type "p", :key nil, :ref nil, :props #js {:children "Two"}, :_owner nil, :_store #js {}}]}, "_owner" nil, "_store" #js {}}

but in my browser console I see the following warning.

Warrning: Each child in a list should have a unique "key" prop.

Check the top-level render call using <div>. See https://reactjs.org/link/warning-keys for more information.
    at p

What's weird is that the old $ function will not display this warning, yet it outputs the exact same object to the REPL.

lilactown commented 1 year ago

@rome-user the fact that the old one didn't show a warning is mysterious. I'm not sure why yet.

It's more mysterious because it doesn't look like in that patch that you update the macro version if $ in core.clj, which the CLJS compiler should prefer. Can you try that and see if it changes the behavior?

rome-user commented 1 year ago

Ah yeah, I forgot to mention I temporarily commented out the $ macro in core.clj so that I could quickly test my changes to the $ function. Some testing in a browser console shows that jsx("div", {:children [jsx("p", {:children "One"}), jsx("p", {:children "One"})]}) indeed prints a warning.

browser console

I'll look further into this and see how Babel etc. compiles things like

<div>
  <p>One</p>
  <p>Two</p>
</div>

rome-user commented 1 year ago

Okay, I've looked at what babel does. Looks like I missed the part of the RFC that specifies the jsxs function to indicate an array was "created by React", and the key requirements seem to go away there.

The example above compiles to

jsxs("div", {children: [jsx("p", {children: "One"}),
            jsx("p", {children: "Two"})]})

During development mode, jsx and jsxs redirect to jsxWithValidationDynamic and jsxWithValidationStatic, respectively.[1] An element generated by jsxs will be treated as "generated by React" and does not require a key when there is an array in the children prop.

In production builds, both jsx and jsxs resolve to the same function.[2]

So to migrate the $ macro to the new JSX transform, we will need to consider the following:

How to distinguish static and dynamic children
How to deal with keys -- at the moment, jsx extracts keys out of props, but upstream intends to stop doing this in the future.[3]

A nice-to-have feature, but not required at all:

Figure out how to expose goog.DEBUG to a macro, so we can emit jsxDEV with __source and __self data. This should allow React devtools to provide slightly more debug information.

I may be able to provide a patch this weekend.

[1] https://github.com/facebook/react/blob/main/packages/react/src/jsx/ReactJSX.js#L16-L19

[2] https://github.com/facebook/react/blob/main/packages/react/src/jsx/ReactJSXElement.js#L210

[3] https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-spreading-key-from-objects

lilactown commented 1 year ago

Awesome write up! Thank you for the thorough look. I look forward to reviewing the patch this weekend.

rome-user commented 1 year ago

I tried refactoring the $ macro to use the modern JSX transform, but I'm still not quite there yet. Here is the changes I've made so far: https://github.com/rome-user/helix/commit/3af9db8

In short, there are several complications that I ran into:

The macro sometimes needs to set a children field in a JS object. I added a 2-arity argument to the props and dom-props macros since that code already does object modification.
nil is a valid React key, so we need custom behavior for (nil? (:key (first args))) and (contains? (first args) :key)
We need to be careful to distinguish strings and arrays in children. I managed to do this, but it feels very hacky and there is too much branching for me.
When there are children, we cannot keep them inside a list, or else CLJS attempts invoking the first child as a function.
The use-effect test in devcards is failing due to the way my modified macro ensures lists are converted to JS arrays
and other ones I cannot think of at the moment

My modifications are very much in a WIP state. There probably will not be a patch finished this weekend, but that's okay. I want to make sure I get everything done not only correctly, but also done well.

lilactown commented 1 year ago

Great work so far. As I was reading your bullets, I had some thoughts

Seems right to do all the object manipulation in the props macros
It's probably right to keep the nil behavior for :key
By "distinguish strings and arrays" do you mean distinguish between children as a single string or children as a seq of element types (including strings)?
this makes sense when emitting the right syntax, and I think your choice of emitting an array constructor is good because it's cheap
Can you elaborate on why that would fail because of changes to the way $ converts lists to arrays?

Very exciting, I appreciate all your work on this!

rome-user commented 1 year ago

By "distinguish strings and arrays" do you mean distinguish between children as a single string or children as a seq of element types (including strings)?

I meant being careful when the child of an element is a string (e.g. <p>abc</p>) and when an element as multiple children. I had to be very careful in where I call cljs.core/array, because invoking it on a string will give results like ["t" "h" "i" "s"], which is not what we want.

Can you elaborate on why that would fail because of changes to the way $ converts lists to arrays?

Sure. At the moment there is a bug in macro where the following happens.

(macroexpand '($ error-boundary ($ effect-test)))
;; => (helix.core/jsxs error-boundary (cljs.core/js-obj "children" (cljs.core/array $ effect-test)))

I'm currently going to try debugging it.

rome-user commented 1 year ago

I made some updates, but I forgot to comment about them here. I made some minor adjustments to my modified $ macro, and now all of the devcards tests work.

Anyway, I do not like how I wrote this region of code.

https://github.com/rome-user/helix/blob/jsx-transform/src/helix/core.clj#L55-L67

I will spend some time later this week on refactoring the macro, then implementing the $ function.

rome-user commented 1 year ago

I have implemented the $ function and added tests for my additions to props and dom-props. I have also compiled some performance benchmarks, using the simple-benchmark card.

The commits on apr 1 implement what I describe above. https://github.com/rome-user/helix/commits/jsx-transform

Here are some benchmarks I just recorded.

Simple benchmark

Renders 10,000 components. All units in milliseconds. Warm-up of 5 iterations. Recorded 10 runs of benchmark.

Before patches

	React	Helix (static)	Helix (dynamic)
Average	86	110	150
Median	87	110	151
Minimum	82	106	147
Maximum	90	113	153

After patches

	React	Helix (static)	Helix (dynamic)
Average	79	106	142
Median	79	107	142
Minimum	76	104	140
Maximum	82	108	145

We see marginal performance improvements!! All that is left is refactor.

EDIT Fixed some rounding mistakes. All figures are now rounded with bias towards (positive) infinity.

rome-user commented 1 year ago

Managed to ooze out extra performance from $ macro and function. Also migrated $d over. I'm still yet to write tests for JSX element renders, and there are still some extra things I want to refactor. Afterwards, I will squash commits and send a PR

lilactown commented 1 year ago

💪🏻 sounds great!

lilactown / helix