Skip to content

Commit b0134f7

Browse files
committed
fix: resolve targetKey collision issue in template generation by using JSON.stringify for tMap; update mental model documentation to reflect the fix and clarify template key generation
1 parent 277e077 commit b0134f7

File tree

7 files changed

+283
-185
lines changed

7 files changed

+283
-185
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@ dist
33
.env
44
.DS_Store
55
sandbox/src/
6+
__screenshots__

docs/MENTAL-MODEL.md

Lines changed: 69 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -878,41 +878,83 @@ t0["class"] = v0; // Fails - "class" is not a valid DOM property
878878

879879
**Status:** Known limitation — documented, workaround available. Future: attribute-to-property translation layer.
880880

881-
### Component with Dynamic Binding Inside Slottable (Under Investigation)
881+
### Template targetKey Collision Bug (Fixed)
882882

883-
**Issue:** A component with dynamic bindings (e.g., `{title}`) used inside another component's slottable crashes at runtime.
883+
**Issue:** Components with child content bindings on root elements could crash at runtime with `TypeError: Cannot read properties of undefined (reading 'data')`.
884884

885-
**Reproduction:**
886-
```jsx
887-
const CardTitle = ({ title }) => (
888-
<h2 class="card-title">{title}</h2>
889-
);
885+
**Root Cause:** The `targetKey` hash in `Template.js` used `Array.join(';')` which flattens nested arrays:
890886

891-
const Card = (props, slottable) => (
892-
<div class="card">{slottable}</div>
893-
);
887+
```javascript
888+
// OLD (buggy):
889+
this.targetKey = this.tMap ? createHash(this.tMap.join(';')) : '';
894890

895-
// This crashes:
896-
<Card>
897-
<CardTitle title="Performance Stats" />
898-
</Card>
891+
// [0].join(';') === "0"
892+
// [[0]].join(';') === "0" // COLLISION!
899893
```
900894

901-
**Error:** `TypeError: Cannot read properties of undefined (reading 'data')` at compose.js line 201 in `clear(anchor)` function.
902-
903-
**Observations:**
904-
- Works: Same pattern in Valhalla tests (components defined inline in same file)
905-
- Fails: wre-dashboards (components imported from separate file)
895+
The `tMap` structure differs based on binding location:
896+
- **Property on child element:** `tMap = [queryIndex]` e.g., `[0]`
897+
- **Child content on root:** `tMap = [[childIndex]]` e.g., `[[0]]`
906898

907-
**Under investigation:** Need to isolate the variable (inline vs imported) with controlled experiment in Valhalla.
899+
When two templates collided on `targetKey`, the vite-plugin's deduplication reused the wrong target function.
908900

909-
**Workaround:** Use imperative DOM manipulation:
901+
**Example collision:**
910902
```jsx
911-
export const CardTitle = ({ title }) => {
912-
const h2 = <h2 class="card-title"></h2>;
913-
h2.textContent = title;
914-
return h2;
915-
};
903+
// Template A: property binding on CHILD element (img)
904+
const Header = () => <header><img src={logo} /></header>;
905+
// tMap: [0] → targets child via querySelectorAll('[data-bind]')[0]
906+
907+
// Template B: child content on ROOT element (h2)
908+
const Title = ({ title }) => <h2>{title}</h2>;
909+
// tMap: [[0]] → targets root's childNodes[0]
910+
911+
// Both hashed to same targetKey!
912+
// If Header loads first, its target function (r,t) => [t[0]]
913+
// gets reused for Title, but Title's h2 has no data-bind attribute,
914+
// so t[0] is undefined → crash
916915
```
917916

918-
**Status:** Open — root cause unknown, investigating.
917+
**Fix:** Use `JSON.stringify()` to preserve array structure:
918+
919+
```javascript
920+
// NEW (fixed):
921+
this.targetKey = this.tMap ? createHash(JSON.stringify(this.tMap)) : '';
922+
923+
// JSON.stringify([0]) === "[0]"
924+
// JSON.stringify([[0]]) === "[[0]]" // Different!
925+
```
926+
927+
**Status:** Fixed (see `packages/thoth/compiler.test.js` "targetKey must distinguish root vs child element bindings" test)
928+
929+
---
930+
931+
## Template Key Hashing Strategy
932+
933+
The compiler generates three hash keys for template deduplication:
934+
935+
### targetKey
936+
Hash of `JSON.stringify(tMap)` — identifies the target function that locates DOM nodes to bind.
937+
938+
**tMap structure:**
939+
- Child/component bindings on root: `[[childIndex]]`
940+
- Child/component bindings on child: `[[queryIndex, childIndex]]`
941+
- Property bindings: `[queryIndex]` (or `-1` for root)
942+
943+
### bindKey
944+
Hash of `bMap.join(';') + propertyNames?.join(';')` — identifies the bind function that applies values.
945+
946+
**bMap structure:**
947+
- `BIND.CHILD (1)` or `BIND.COMPONENT (2)`: positive number (binding type)
948+
- `BIND.PROP (3)`: negative number (index into propertyNames × -1)
949+
950+
### Template ID
951+
Hash of `html + bindKey + targetKey` — unique identifier for the entire template.
952+
953+
### Future Considerations
954+
955+
The current approach duplicates property names across templates. For large projects with many elements using common properties (e.g., `className`, `onClick`), this could be optimized:
956+
957+
1. **Property name interning:** Use indexes into a shared property name table
958+
2. **Trade-off:** Smaller payload vs. extra indirection at runtime
959+
960+
The bindKey already uses indexes for property names within a template, but doesn't share across templates. Whether this optimization is worthwhile depends on actual payload sizes in production builds.

0 commit comments

Comments
 (0)