adamhaile / surplus

High performance JSX web views for S.js applications
639 stars 26 forks source link

Support for arrays in className #90

Closed Qix- closed 3 years ago

Qix- commented 3 years ago

As I understand it, className only accepts a string. Would it be possible to add an array form, too?

Currently, the problem is that, when a signal is used as one of multiple class names (especially when using CSS-in-JS, such as JXX), you usually have to write something like this:

const someSignal = S.value(false);
const C = css({ classname1: {...}, classname2: {...} });

<Foo className={`${C.classname1} {someSignal() && C.classname2}`} />

which, on top of being somewhat messy syntax, evaluates to classname1 false if the second signal is falsey, when ideally it'd be simply classname1.

Being able to specify an array would look like this:

const someSignal = S.value(false);
const C = css({ classname1: {...}, classname2: {...} });

<Foo className={[C.classname1, someSignal() && C.classname2]} />

And then, under the hood, you'd be able to do the equivalent of elem.classList.add(...props.className.filter(Boolean)).

Further, if there's a concern about maintaining parity with React's JSX, perhaps introduce an extended mode that includes Surplus-specific extensions that is imported with import * as Surplus from 'surplus/ex'?

Qix- commented 3 years ago

I got this far, but I think there's something missing in the compiler/codegen bit that results in classes not getting added (at least in my test case). No errors, just some classes are missing.

Based on 2aca5a36ceb6a7cbb4d609cd04ee631714602f91.

diff --git a/src/compiler/fieldData.ts b/src/compiler/fieldData.ts
index 238fb45..a78d4d2 100644
--- a/src/compiler/fieldData.ts
+++ b/src/compiler/fieldData.ts
@@ -3,10 +3,11 @@
 export const enum FieldFlags {
     // bottom 2 bits encode field type
     Type      = 3,
+
     Property  = 0,
     Attribute = 1,
     Ignore    = 2,
-    Assign    = 3
+    Special   = 3  // special handling
 }

 export type FieldData = [ string, string | null, FieldFlags ];
@@ -15,11 +16,12 @@ const
     // pre-seed the caches with a few special cases, so we don't need to check for them in the common cases
     htmlFieldCache = {
         // special props
-        style           : [ 'style'          , null, FieldFlags.Assign    ],
+        style           : [ 'style'          , null, FieldFlags.Special   ],
         ref             : [ 'ref'            , null, FieldFlags.Ignore    ],
         fn              : [ 'fn'             , null, FieldFlags.Ignore    ],
         // attr compat
-        class           : [ 'className'      , null, FieldFlags.Property  ],
+        class           : [ 'className'      , null, FieldFlags.Special   ],
+        className       : [ 'className'      , null, FieldFlags.Special   ],
         for             : [ 'htmlFor'        , null, FieldFlags.Property  ],
         "accept-charset": [ 'acceptCharset'  , null, FieldFlags.Property  ],
         "http-equiv"    : [ 'httpEquiv'      , null, FieldFlags.Property  ],
@@ -36,7 +38,7 @@ const
     } as { [ field : string ] : FieldData },
     svgFieldCache = {
         // special props
-        style        : [ 'style'      , null, FieldFlags.Assign    ],
+        style        : [ 'style'      , null, FieldFlags.Special   ],
         ref          : [ 'ref'        , null, FieldFlags.Ignore    ],
         fn           : [ 'fn'         , null, FieldFlags.Ignore    ],
         // property compat
@@ -154,4 +156,4 @@ export const
         let data = attr ? buildAttrData(name) : buildPropData(name);

         return cache[field] = data;
-    }
\ No newline at end of file
+    }
diff --git a/src/runtime/dom.ts b/src/runtime/dom.ts
index e88910f..6ba247c 100644
--- a/src/runtime/dom.ts
+++ b/src/runtime/dom.ts
@@ -1,15 +1,17 @@
+import { reduceClassString } from './spread';
+
 const svgNS = "http://www.w3.org/2000/svg";

-export function createElement(tag : string, className : string | null, parent : HTMLElement | null) {
+export function createElement(tag : string, className : string[] | string | null, parent : HTMLElement | null) {
     var el = document.createElement(tag);
-    if (className) el.className = className;
+    if (className) el.className = Array.isArray(className) ? reduceClassString(className) : className;
     if (parent) parent.appendChild(el);
     return el;
 }

-export function createSvgElement(tag : string, className : string | null, parent : HTMLElement | null) {
+export function createSvgElement(tag : string, className : string[] | string | null, parent : HTMLElement | null) {
     var el = document.createElementNS(svgNS, tag);
-    if (className) el.setAttribute("class", className);
+    if (className) el.setAttribute("class", Array.isArray(className) ? reduceClassString(className) : className);
     if (parent) parent.appendChild(el);
     return el;
 }
diff --git a/src/runtime/fieldData.ts b/src/runtime/fieldData.ts
index 238fb45..a78d4d2 100644
--- a/src/runtime/fieldData.ts
+++ b/src/runtime/fieldData.ts
@@ -3,10 +3,11 @@
 export const enum FieldFlags {
     // bottom 2 bits encode field type
     Type      = 3,
+
     Property  = 0,
     Attribute = 1,
     Ignore    = 2,
-    Assign    = 3
+    Special   = 3  // special handling
 }

 export type FieldData = [ string, string | null, FieldFlags ];
@@ -15,11 +16,12 @@ const
     // pre-seed the caches with a few special cases, so we don't need to check for them in the common cases
     htmlFieldCache = {
         // special props
-        style           : [ 'style'          , null, FieldFlags.Assign    ],
+        style           : [ 'style'          , null, FieldFlags.Special   ],
         ref             : [ 'ref'            , null, FieldFlags.Ignore    ],
         fn              : [ 'fn'             , null, FieldFlags.Ignore    ],
         // attr compat
-        class           : [ 'className'      , null, FieldFlags.Property  ],
+        class           : [ 'className'      , null, FieldFlags.Special   ],
+        className       : [ 'className'      , null, FieldFlags.Special   ],
         for             : [ 'htmlFor'        , null, FieldFlags.Property  ],
         "accept-charset": [ 'acceptCharset'  , null, FieldFlags.Property  ],
         "http-equiv"    : [ 'httpEquiv'      , null, FieldFlags.Property  ],
@@ -36,7 +38,7 @@ const
     } as { [ field : string ] : FieldData },
     svgFieldCache = {
         // special props
-        style        : [ 'style'      , null, FieldFlags.Assign    ],
+        style        : [ 'style'      , null, FieldFlags.Special   ],
         ref          : [ 'ref'        , null, FieldFlags.Ignore    ],
         fn           : [ 'fn'         , null, FieldFlags.Ignore    ],
         // property compat
@@ -154,4 +156,4 @@ export const
         let data = attr ? buildAttrData(name) : buildPropData(name);

         return cache[field] = data;
-    }
\ No newline at end of file
+    }
diff --git a/src/runtime/spread.ts b/src/runtime/spread.ts
index 652628e..cd906ca 100644
--- a/src/runtime/spread.ts
+++ b/src/runtime/spread.ts
@@ -4,6 +4,10 @@ import { setAttributeNS } from './index';

 export type PropObj = { [ name : string ] : any };

+export function reduceClassString(classes: string[]) {
+    return classes.reduce((acc, s) => s ? acc + ' ' + s : acc, '').slice(1);
+}
+
 export function assign(a : PropObj, b : PropObj) {
     var props = Object.keys(b);
     for (var i = 0, len = props.length; i < len; i++) {
@@ -29,7 +33,16 @@ function setField(node : HTMLElement | SVGElement, field : string, value : any,
     } else if (type === FieldFlags.Attribute) {
         if (namespace) setAttributeNS(node, namespace, name, value);
         else setAttribute(node, name, value);
-    } else if (type === FieldFlags.Assign) {
-        if (value && typeof value === 'object') assign(node.style, value);
+    } else if (type === FieldFlags.Special) {
+        switch (name) {
+        case 'style':
+            if (value && typeof value === 'object') assign(node.style, value);
+            break;
+        case 'className':
+            if (value && Array.isArray(value)) value = reduceClassString(value);
+            if (namespace) node = (node as any)[namespace];
+            (node as any).className = value;
+            break;
+        }
     }
 }
adamhaile commented 3 years ago

Sorry for slow reply.

My first thought is that this might be something better done in client code or maybe in a fn mixin. So options like:

// using .join(" ")
<Foo className={[C.classname1, someSignal() && C.classname2].join(" ")} />
// using your reduceClassString to avoid the " false " classes:
<Foo className={reduceClassString([C.classname1, someSignal() && C.classname2])} />
// using a `fn` mixin
<Foo fn={class([C.classname1, someSignal() && C.classname2])} />

Doing it in the library means any time we're setting className we need to check for the presence of an array. That's a very hot path, and last I checked, Array.isArray() had a non-trivial cost, especially with the speed that Surplus runs.

If it were to happen in the library, it might be better as the classList property, as its DOM semantics are closer than className. So far, though, I've avoided extending Surplus' JSX semantics beyond React and HTML compatibility. I err on the conservative side with that, since once its in the library, I need a very good reason to change it, as it may break someone's code. So I need to be extra sure it's the right thing to add, and I don't have that clarity around a lot of possibly-useful extensions.

Qix- commented 3 years ago

The only problem with .join(' ') is that someSignal && C.classname2 will result in "class-name1 false" or "class-name1 undefined" or something of that sort, which is mostly what I'm trying to get away from.

Yes, you can do [C.classname1, someSignal() && C.classname2].filter(Boolean).join(' ') but for non-trivial lists of classes and signals and the like it becomes unreadable quite quickly.

I agree about the Array.isArray() bit and I didn't like modifying the hotpath either. classList seems like a much better option.


As for extending JSX beyond React's compatibility, why not introduce classList and hide it behind import * as Surplus from 'surplus/ex'? You could put incompatibilities in there and code would opt-in to it. Just a matter of packaging an ex.js script in the top level of the published package that extends the runtime or compiler however is necessary.

I have a number of applications that are first-class Surplus citizens that I never intend to be touched by anything related to React - being able to opt-in to more powerful features at the "expense" of incompatbility with React would be very useful.

Qix- commented 3 years ago

Going to close this - I ended up finding a fine solution after all.

My CSS via jss wrapper normally just returned a flat object with the generated class names, so I just instead crafted a copy of a reduction utility function and added the class names onto it instead.

// NOTE: pseudocode
export default cssSpec => Object.assign(
    (...args) => args.filter(Boolean).join(' '),
    jss(cssSpec)
);
const C = css({
    wrapper: { display: 'flex' }
});

export default ({className, children}) => (<div className={C(C.wrapper, className)}>{children}</div>);