Allow `ax()` to handle non-string classnames when collected. #1696

kylorhall-atlassian · 2024-07-18T02:42:52Z

I want to allow our ax() collection to better handle arbitrary values. The example being passing using our pass-through styles with cssMap like <Component xcss={cx(…)} /> or <Component xcss={[…]} /> results in ax capturing things like this:

[
  '_1bah1yb4',
  undefined,
  ['_16jlkb7n', undefined],
  AtomicGroups {
    values: Map(2) { '_1bsb' => '_1bsb1osq', '_16jl' => '_16jlkb7n' }
  }
]

I don't know if it's the best way to handle this, but internally we've just been invoking toString() via a wrapper, and we're trying to get to native Compiled. Makes sense (and works) to me!

PR checklist

I have...

Updated or added applicable tests
Chosen to not document this.

The example being passing `<Component xcss={cx(…)} />` or `<Component xcss={[…]} />` results in `ax` capturing things like this: ```tsx [ '_1bah1yb4', undefined, ['_16jlkb7n', undefined], AtomicGroups { values: Map(2) { '_1bsb' => '_1bsb1osq', '_16jl' => '_16jlkb7n' } } ] ``` I don't know if it's the best way to handle this, but internally we've just been invoking `toString()` via a wrapper, and we're trying to get to native Compiled. Makes sense (and works) to me!

changeset-bot · 2024-07-18T02:42:56Z

🦋 Changeset detected

Latest commit: 7835fd8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@compiled/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2024-07-18T02:43:08Z

✅ Deploy Preview for compiled-css-in-js canceled.

Name	Link
🔨 Latest commit	`7835fd8`
🔍 Latest deploy log	https://app.netlify.com/sites/compiled-css-in-js/deploys/6698946c7fa37b0008030827

kylorhall-atlassian · 2024-07-18T02:45:04Z

packages/react/src/runtime/ax.ts

-export default function ax(classNames: (string | undefined | null | false)[]): string | undefined {
-  if (classNames.length <= 1 && (!classNames[0] || classNames[0].indexOf(' ') === -1)) {
+export default function ax(
+  classNames: (string | undefined | null | false | AtomicGroups)[]


With the introduction of cx, I believe this is the proper type as this does happen (or cx should change in a way I don't know how).

Realistically this could also be infinitely recursive, I don't think anything flattens here…but I won't solve a problem that doesn't really exist.

itsdouges · 2024-07-18T02:45:29Z

packages/react/src/runtime/ax.ts

@@ -41,7 +48,16 @@ export default function ax(classNames: (string | undefined | null | false)[]): s
      continue;
    }

-    const groups = cls.split(' ');


What if:

const groups = (cls + '').split(' ');

Yeah actually, doing const groups = String(cls).split(' '); does work actually, just feels like it doesn't tell you what it's doing is all.

I'd do whatever results in better performance.

Well, not sure about the array syntax <Component xcss={[style, style]} />, but whatever, that fails types anyways.

Doing arbitrary online benchmarks which mean little, protecting via a typeof like:

if (typeof cls === 'string') { return cls.split(' '); } else { return String(cls).split(' '); }

is faster by ~4% than just blindly calling String(c1) first (and if we add Array.isArray() in there it's much faster). I gather String() doesn't handle this (but could/should), it's an implementation detail that may not be consistent.

Worst-case, I'd expect it to be 1:1, I don't see how v8 could do anything super special here—basically the negative impact is just from doing typeof checks, which are pretty minimal.

Sweet ok. I think we can lean on type assumptions and safely use the typeof check tbh.

kylorhall-atlassian · 2024-07-18T02:47:06Z

packages/react/src/runtime/ax.ts

Could use help actually figuring out how to test this change as it's apparent the scenario doesn't work, or I'm doing something wrong. I'm not sure how our <Button xcss={cx(…)} /> tests are passing (I figured this wasn't tested, but it is? I expected those to error…)

At least I tested this in ax.test.ts now.

kylorhall-atlassian · 2024-07-18T03:54:20Z

packages/react/src/runtime/ax.ts

+    } else if (Array.isArray(cls)) {
+      groups = cls; // NOTE: Not really a valid scenario, so we don't deeply recurse or handle this properly
+    } else {
+      // NOTE: Could throw an error here if this isn't our `AtomicGroups` to help understand that this is collecting invalid styles.


I found doing a cls instanceof AtomicGroups wasn't consistently working, might as well try to blanket support, though…it does mean previous things that would throw 'cls.split is not a function' now more quietly fail.

Yeah that's fine. We can lean on the typesystem for everything else.

somewhat unrelated question, but do you guys use ac and AtomicGroups for anything? i thought that was part of the class name compression work that we've now shelved (probably safe to just yeet)

So the root example comes from using cx() which uses ac(). Frankly, I do not know anything here, I'm just hacking it together and I think I understand what's going on.

I think we used ac based on @liamqma's guidance. If you think it's not useful we can switch it out for whatever.

Yeah, we've parked classname compression.
I suggest switching it out for the old good ax. 🙂

No need forax() to handle AtomicGroup. less runtime code 🙂

oops i was going to write a big comment but it got deleted 😢

but yeah depends on whether you want to support nesting like cx([cx('_aaaa_a'), '_aaaa_b'])... if not, i guess there's no value that an ac-based approach would provide

(and maybe delete the ac/class name compression stuff altogether too, so we don't have unused functions in runtime? but that's probably more suited for a follow-up PR)

Makes sense to me, is it literally as simple as swapping out ac for ax? It kinda looks like it locally as I think I've got all my tests passing 🙈

-return ac(actualStyles); +return ax(actualStyles);

If that's a drop-in, I can pop that out tomorrow, but I won't make the whole removal (bit over my head).

I think this works: #1697

dddlr · 2024-07-18T04:42:15Z

packages/react/src/runtime/ax.ts

+    if (typeof cls === 'string') {
+      groups = cls.split(' ');
+    } else if (Array.isArray(cls)) {
+      groups = cls; // NOTE: Not really a valid scenario, so we don't deeply recurse or handle this properly


this is an area in the codebase i haven't looked at in a while, so i'm not too familiar with the runtime stuff 😅

from the comment and the tests, i take it only APIs outside of Compiled (and never Compiled APIs) would ever create a situation where Array.isArray(cls) is true right? (e.g. className = [['_abcd1234', '_efgh1234'], '_ijkl1234 mnop1234', ...])

i don't have a preference on whether to keep the else if (Array.isArray(cls)) { ... } branch in or not, just aiming to understand the PR fully

Just to drop the scenario, right now let's say if you do like this (passing props.xcss around):

const Component = (props) => ( <div className={xcss} css={{ margin: 0 }} /> ); export default (props) => <Component xcss={[{ margin: '12px' }, props.xcss]} />;

You will get a type error because our xcss implementation requires xcss={cx({ margin: '12px' }, props.xcss)} and arrays won't work, but in theory I could see people copy + pasting css={[…]} array into xcss={[…]} and ignoring it and it throwing an error (because I kind of did that 🤣).

I don't mind much either way to be honest, it does have a type error.

I would prefer to either throw new Error(…) in here or keep it, because if it does get passed, doing String(array).split(' '); is quite unperformant (like 50–100% reduction in executions per minute if we attempt to stringify arrays, I wouldn't want that hitting production).

that makes sense, thank you for the additional context

kylorhall-atlassian · 2024-07-19T01:31:00Z

Closing as I think #1697 is just a better approach.

kylorhall-atlassian changed the title ~~Allow ax() to handle non-string classnames when collected.~~ IDEA: Allow ax() to handle non-string classnames when collected. Jul 18, 2024

kylorhall-atlassian commented Jul 18, 2024

View reviewed changes

itsdouges reviewed Jul 18, 2024

View reviewed changes

kylorhall-atlassian commented Jul 18, 2024

View reviewed changes

Test and make this a bit more resilient from real-world testing

48904f5

kylorhall-atlassian commented Jul 18, 2024

View reviewed changes

kylorhall-atlassian changed the title ~~IDEA: Allow ax() to handle non-string classnames when collected.~~ Allow ax() to handle non-string classnames when collected. Jul 18, 2024

kylorhall-atlassian marked this pull request as ready for review July 18, 2024 03:54

kylorhall-atlassian requested review from JakeLane, pancaspe87, liamqma and dddlr as code owners July 18, 2024 03:54

kylorhall-atlassian added 2 commits July 18, 2024 16:02

Increase bytesize for ax following support for AtomicGroups classes

649820e

Add changeset

7835fd8

dddlr reviewed Jul 18, 2024

View reviewed changes

kylorhall-atlassian mentioned this pull request Jul 18, 2024

Fix the cx() function's class collection at runtime as it generated class instances rather than strings. #1697

Merged

2 tasks

kylorhall-atlassian closed this Jul 19, 2024

kylorhall-atlassian deleted the handle-non-string-classnames branch October 23, 2024 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `ax()` to handle non-string classnames when collected. #1696

Allow `ax()` to handle non-string classnames when collected. #1696

kylorhall-atlassian commented Jul 18, 2024 •

edited

Loading

changeset-bot bot commented Jul 18, 2024 •

edited

Loading

netlify bot commented Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024

itsdouges Jul 18, 2024

kylorhall-atlassian Jul 18, 2024

itsdouges Jul 18, 2024

kylorhall-atlassian Jul 18, 2024

kylorhall-atlassian Jul 18, 2024 •

edited

Loading

itsdouges Jul 18, 2024

kylorhall-atlassian Jul 18, 2024

kylorhall-atlassian Jul 18, 2024

kylorhall-atlassian Jul 18, 2024

itsdouges Jul 18, 2024

dddlr Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024 •

edited

Loading

itsdouges Jul 18, 2024

liamqma Jul 18, 2024 •

edited

Loading

dddlr Jul 18, 2024

dddlr Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024

dddlr Jul 18, 2024

kylorhall-atlassian Jul 18, 2024 •

edited

Loading

dddlr Jul 18, 2024

kylorhall-atlassian commented Jul 19, 2024

Allow ax() to handle non-string classnames when collected. #1696

Allow ax() to handle non-string classnames when collected. #1696

Conversation

kylorhall-atlassian commented Jul 18, 2024 • edited Loading

PR checklist

changeset-bot bot commented Jul 18, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Jul 18, 2024 • edited Loading

✅ Deploy Preview for compiled-css-in-js canceled.

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

kylorhall-atlassian Jul 18, 2024 • edited Loading

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

dddlr Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

kylorhall-atlassian Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamqma Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddlr Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

kylorhall-atlassian Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylorhall-atlassian Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylorhall-atlassian commented Jul 19, 2024

Allow `ax()` to handle non-string classnames when collected. #1696

Allow `ax()` to handle non-string classnames when collected. #1696

kylorhall-atlassian commented Jul 18, 2024 •

edited

Loading

changeset-bot bot commented Jul 18, 2024 •

edited

Loading

netlify bot commented Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024 •

edited

Loading

dddlr Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024 •

edited

Loading

liamqma Jul 18, 2024 •

edited

Loading

dddlr Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024 •

edited

Loading

kylorhall-atlassian Jul 18, 2024 •

edited

Loading