[SPIKE] smart ax with dynamic length and prefix family matching#1902
[SPIKE] smart ax with dynamic length and prefix family matching#1902xtan-atlas wants to merge 3 commits into
Conversation
|
✅ Deploy Preview for compiled-css-in-js canceled.
|
| '_aaaaeeee', | ||
| ], | ||
| [ | ||
| 'should treat non-standard shorter atomic-looking class names consistently with suffix parsing', |
There was a problem hiding this comment.
This was failing, because original test was accepting 8 char class. It works before due to it treat first 4 char as group, but after we change logic to slice(className.length - valueLength), the group hash for 8 char string would be first 3 char...
| for (const existingKey in map) { | ||
| if (key.startsWith(existingKey)) { | ||
| delete map[existingKey]; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm a bit confused on what this does. So let's say we have two: ax(['_abcd1234', '_abcde2345']); (old and new hashes)
_abcd1234runs, setsmap = { '_abcd': '_abcd1234' };_abcd1234runs, deletes that map entry, setsmap = { '_abcde': '_abcde2345' };
That makes sense, you want the second value with that hash…
But then let's invert it: ax(['_abcde1234', '_abcd2345']); don't you get map = { '_abcd': '_abcd1234', '_abcde': '_abcde2345' };?
Is that a problem?
| if (isAtomic) { | ||
| removePrefixFamilyEntries(key); | ||
| } |
There was a problem hiding this comment.
I think a comment (maybe in above jsdoc) explaining this part is helpful, especially RE: my comment above as I think this might get more complex…
| 'should allow a variable-length longer group to override its legacy prefix family', | ||
| ['_aaaabbbb', '_aaaa12cccc'], | ||
| '_aaaa12cccc', |
There was a problem hiding this comment.
Make sure we have a flipped version of this test as well, or like all three in various orders, whatever seems sensible.
There was a problem hiding this comment.
added more flipped tests
| * ever needs to be disabled. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const setAtomicClassInGroupMapSimpleScan = (groupKey: string, className: string) => { |
There was a problem hiding this comment.
this is much easier to understand but a bit slower when ax large amount of classes, like Editor styles...
| * For runtime performance, the bucket is mutated in place using swap-and-pop | ||
| * so we avoid allocating a fresh array on every insert. | ||
| */ | ||
| const setAtomicClassInGroupMapBucketed = (groupKey: string, className: string) => { |
There was a problem hiding this comment.
This is hard to understand, but more performant. But still slower than without legacy group hash support. It's like slower 20-40%. So backwards support do come with cost.
What is this change?
Make
axsupports longer classes like_ggggggvvvv, and can detect group hash isgggggg.Support overrides legacy short classnames:
ax('_ggggvvvv', '_ggggttvvvv') => '_ggggttvvvv'Why are we making this change?
The 4 char group hash is too short, cause class collision and drop styles silently.
PR checklist
Don't delete me!
I have...
website/