Merge pull request #66 from k0d13/kodie/jsx-placeholder-labels

Add say-tag for naming JSX placeholders

Author: k0d13

.changeset/lucky-cases-shake.md

@@ -0,0 +1,7 @@
+---
+"@saykit/transform-jsx": minor
+"@saykit/config": minor
+"@saykit/react": minor
+---
+
+Add `say-tag` to name JSX placeholders, extract childless elements as self-closing tags, and compile message values behind an underscore so no name is reserved

examples/react/src/components/task-card.tsx

@@ -31,13 +31,16 @@ export function TaskCard({ task }: { task: Task }) {
 
       <p className="card__meta">
         {/*
-          A JSX child inside `<Say>` is extracted as a positional `<0>` tag, so a
-          translator can move the emphasised name anywhere in the sentence
-          without touching markup. The element itself never enters the catalogue.
+          A JSX child inside `<Say>` is extracted as a tag, so a translator can
+          move the emphasised name anywhere in the sentence without touching
+          markup. The element itself never enters the catalogue. `say-tag`
+          names that tag `<bold>` instead of a positional `<0>` — describe what
+          the tag does, not what it wraps. The attribute is stripped at build
+          time and never reaches the DOM.
         */}
         {task.assignee ? (
           <Say>
-            Assigned to <strong>{task.assignee}</strong>
+            Assigned to <strong say-tag="bold">{task.assignee}</strong>
           </Say>
         ) : (
           <Say>Nobody is on this yet</Say>

examples/react/src/locales/en.po

@@ -12,7 +12,7 @@ msgstr ""
 "Plural-Forms: \n"
 "X-Generator: saykit\n"
 
-#: src/components/task-card.tsx:53
+#: src/components/task-card.tsx:56
 msgid ""
 "{0, plural,\n"
 "  =0 {All subtasks done}\n"
@@ -38,7 +38,7 @@ msgstr ""
 "  other {# days overdue}\n"
 "}"
 
-#: src/components/task-card.tsx:63
+#: src/components/task-card.tsx:66
 msgid ""
 "{0, plural,\n"
 "  one {# unresolved comment}\n"
@@ -108,9 +108,9 @@ msgstr ""
 "  other {# tasks still open}\n"
 "}"
 
-#: src/components/task-card.tsx:39
-msgid "Assigned to <0>{1}</0>"
-msgstr "Assigned to <0>{1}</0>"
+#: src/components/task-card.tsx:42
+msgid "Assigned to <bold>{0}</bold>"
+msgstr "Assigned to <bold>{0}</bold>"
 
 #: src/components/task-card.tsx:9
 msgid "Due today"
@@ -120,7 +120,7 @@ msgstr "Due today"
 msgid "Language"
 msgstr "Language"
 
-#: src/components/task-card.tsx:43
+#: src/components/task-card.tsx:46
 msgid "Nobody is on this yet"
 msgstr "Nobody is on this yet"
 

packages/config/src/features/messages/convert.test.ts

@@ -29,6 +29,22 @@ describe('convertMessageToIcu', () => {
       .toMatchInlineSnapshot('"<0>Hello world!</0>"');
   });
 
+  it('should generate childless element messages as self-closing', () => {
+    const message = new ElementMessage('icon', [], dummy);
+    expect(convertMessageToIcu(message)) //
+      .toMatchInlineSnapshot('"<icon/>"');
+  });
+
+  it('should keep element messages paired when a child renders to nothing', () => {
+    const message = new ElementMessage(
+      'bold',
+      [new CompositeMessage({}, [], [], [], dummy)],
+      dummy,
+    );
+    expect(convertMessageToIcu(message)) //
+      .toMatchInlineSnapshot('"<bold></bold>"');
+  });
+
   it('should generate choice messages with numeric identifiers as `=n`', () => {
     const message = new ChoiceMessage(
       'plural',

packages/config/src/features/messages/convert.ts

@@ -17,6 +17,11 @@ export function convertMessageToIcu(message: Message) {
         return `{${String(message.identifier)}}`;
 
       case message instanceof ElementMessage: {
+        // A childless element is self-closing, so a translator has nowhere to
+        // insert content that the element never expected. This tracks the
+        // source: an element written as a pair stays a pair, even when its
+        // children happen to render to nothing.
+        if (message.children.length === 0) return `<${String(message.identifier)}/>`;
         const children = message.children.map((m) => internalConvertMessageToIcu(m)).join('');
         return `<${String(message.identifier)}>${children}</${String(message.identifier)}>`;
       }

packages/config/src/features/messages/identifier.test.ts

@@ -64,6 +64,77 @@ describe('assignSequenceIdentifiers', () => {
     expect(value.identifier).toBe('0');
   });
 
+  it('skips sequence numbers already taken by an explicit identifier', () => {
+    const tagged = new ElementMessage('0', [], null);
+    const auto = new ArgumentMessage(AUTO_INCREMENT_IDENTIFIER, null);
+    assignSequenceIdentifiers(new CompositeMessage({}, [], [], [tagged, auto], null));
+    expect(tagged.identifier).toBe('0');
+    expect(auto.identifier).toBe('1');
+  });
+
+  it('throws when two elements share a tag', () => {
+    const message = new CompositeMessage(
+      {},
+      [],
+      [],
+      [new ElementMessage('link', [], null), new ElementMessage('link', [], null)],
+      null,
+    );
+    expect(() => assignSequenceIdentifiers(message)).toThrow(
+      "Duplicate element tag 'link', give each element in a message its own tag unless they are identical",
+    );
+  });
+
+  it('allows two elements to share a tag when they are equivalent', () => {
+    const message = new CompositeMessage(
+      {},
+      [],
+      [],
+      [new ElementMessage('link', [], 'a'), new ElementMessage('link', [], 'a')],
+      null,
+    );
+    expect(() =>
+      assignSequenceIdentifiers(message, { current: 0 }, (a, b) => a === b),
+    ).not.toThrow();
+  });
+
+  it('throws when elements sharing a tag are not equivalent', () => {
+    const message = new CompositeMessage(
+      {},
+      [],
+      [],
+      [new ElementMessage('link', [], 'a'), new ElementMessage('link', [], 'b')],
+      null,
+    );
+    expect(() => assignSequenceIdentifiers(message, { current: 0 }, (a, b) => a === b)).toThrow(
+      "Duplicate element tag 'link'",
+    );
+  });
+
+  it('allows two arguments to share an identifier', () => {
+    const message = new CompositeMessage(
+      {},
+      [],
+      [],
+      [new ArgumentMessage('total', null), new ArgumentMessage('total', null)],
+      null,
+    );
+    expect(() => assignSequenceIdentifiers(message)).not.toThrow();
+  });
+
+  it('throws when an element tag collides with an argument', () => {
+    const message = new CompositeMessage(
+      {},
+      [],
+      [],
+      [new ElementMessage('total', [], null), new ArgumentMessage('total', null)],
+      null,
+    );
+    expect(() => assignSequenceIdentifiers(message)).toThrow(
+      "Element tag 'total' collides with an argument of the same name",
+    );
+  });
+
   it('leaves a literal message unchanged', () => {
     const literal = new LiteralMessage('hi');
     expect(() => assignSequenceIdentifiers(literal)).not.toThrow();

packages/config/src/features/messages/identifier.ts

@@ -8,20 +8,83 @@ import {
 
 export const AUTO_INCREMENT_IDENTIFIER = Symbol('auto-increment');
 
-export function assignSequenceIdentifiers(message: Message, sequence = { current: 0 }) {
-  if (
-    message instanceof ArgumentMessage ||
-    message instanceof ElementMessage ||
-    message instanceof ChoiceMessage
-  )
-    if (message.identifier === AUTO_INCREMENT_IDENTIFIER)
-      message.identifier = `${sequence.current++}`;
-  if (message instanceof CompositeMessage || message instanceof ElementMessage)
-    for (const child of message.children) assignSequenceIdentifiers(child, sequence);
-  if (message instanceof ChoiceMessage)
-    for (const branch of message.branches) {
-      if (branch.identifier === AUTO_INCREMENT_IDENTIFIER)
-        branch.identifier = `${sequence.current++}`;
-      assignSequenceIdentifiers(branch.value, sequence);
+/**
+ * Decides whether two elements sharing a tag are the same element, and so may
+ * share it. Only the syntax that produced them can answer that, so the caller
+ * supplies the comparison; by default no two elements are interchangeable.
+ */
+export type ElementEquivalence = (a: any, b: any) => boolean;
+
+export function assignSequenceIdentifiers(
+  message: Message,
+  sequence = { current: 0 },
+  equivalent: ElementEquivalence = () => false,
+) {
+  const reserved = collectAssignedIdentifiers(message, equivalent);
+
+  function next() {
+    let identifier = `${sequence.current++}`;
+    while (reserved.has(identifier)) identifier = `${sequence.current++}`;
+    return identifier;
+  }
+
+  function walk(message: Message) {
+    if (
+      message instanceof ArgumentMessage ||
+      message instanceof ElementMessage ||
+      message instanceof ChoiceMessage
+    )
+      if (message.identifier === AUTO_INCREMENT_IDENTIFIER) message.identifier = next();
+    if (message instanceof CompositeMessage || message instanceof ElementMessage)
+      for (const child of message.children) walk(child);
+    if (message instanceof ChoiceMessage)
+      for (const branch of message.branches) {
+        if (branch.identifier === AUTO_INCREMENT_IDENTIFIER) branch.identifier = next();
+        walk(branch.value);
+      }
+  }
+
+  walk(message);
+}
+
+/**
+ * Collect the identifiers already assigned before this pass runs, so generated
+ * sequence numbers never shadow an explicit one (e.g. an element tagged `0`).
+ *
+ * Elements that differ need their own tag: they each compile to their own prop,
+ * and a translator has to be able to tell them apart. Repeats are fine when
+ * nothing distinguishes them — two identical elements are one prop, the same
+ * way the same variable interpolated twice is one value — so arguments are
+ * never checked and elements only when they are not equivalent.
+ */
+function collectAssignedIdentifiers(message: Message, equivalent: ElementEquivalence) {
+  const tags = new Map<string, ElementMessage>();
+  const values = new Set<string>();
+
+  function walk(message: Message) {
+    if (message instanceof ElementMessage && typeof message.identifier === 'string') {
+      const previous = tags.get(message.identifier);
+      if (previous && !equivalent(previous.expression, message.expression))
+        throw new Error(
+          `Duplicate element tag '${message.identifier}', give each element in a message its own tag unless they are identical`,
+        );
+      tags.set(message.identifier, message);
     }
+    if (
+      (message instanceof ArgumentMessage || message instanceof ChoiceMessage) &&
+      typeof message.identifier === 'string'
+    )
+      values.add(message.identifier);
+    if (message instanceof CompositeMessage || message instanceof ElementMessage)
+      for (const child of message.children) walk(child);
+    if (message instanceof ChoiceMessage) for (const branch of message.branches) walk(branch.value);
+  }
+
+  walk(message);
+
+  for (const tag of tags.keys())
+    if (values.has(tag))
+      throw new Error(`Element tag '${tag}' collides with an argument of the same name`);
+
+  return new Set([...tags.keys(), ...values]);
 }

packages/integration-react/src/runtime/index.test.ts

@@ -29,7 +29,7 @@ describe('Say', () => {
     const element = (Say as (p: unknown) => ReactElement)({
       id: 'greet',
       whitespace: false,
-      name: bold,
+      _name: bold,
     });
 
     expect(element.type).toBe(Renderer);
@@ -40,21 +40,47 @@ describe('Say', () => {
     };
     expect(props.html).toBe('<name/> world');
     expect(props.whitespace).toBe(false);
-    // The descriptor passed to `say.call` has JSX-safe keys resolved and the
-    // `whitespace` prop removed.
-    expect(call).toHaveBeenCalledWith(expect.objectContaining({ id: 'greet', name: bold }));
+    // The descriptor passed to `say.call` is exactly the message id and the
+    // value props unprefixed, so a prefixed key or a renderer prop leaking
+    // into it fails here rather than reaching the runtime.
+    expect(call).toHaveBeenCalledWith({ id: 'greet', name: bold });
 
     // A slot that maps to a valid element clones it; other slots pass the tag through.
     const resolved = props.components('name') as (p: object) => ReactElement;
     expect(typeof resolved).toBe('function');
     expect(isValidElement(resolved({}))).toBe(true);
     expect(props.components('missing')).toBe('missing');
-    // `id` is a string, not an element, so it also passes through as the tag.
+    // The message id is not a value, so nothing resolves for it as a tag.
     expect(props.components('id')).toBe('id');
     // Called with no tag, it returns the (undefined) tag.
     expect(props.components()).toBeUndefined();
   });
 
+  it('lets a value be named after one of Say’s own props', () => {
+    const call = vi.fn(() => '<id/> and <whitespace/>');
+    globalThis.GET_SAY = () => ({ call });
+
+    const bold = createElement('b', null, 'Ada');
+    const element = (Say as (p: unknown) => ReactElement)({
+      id: 'greet',
+      whitespace: false,
+      _id: bold,
+      _whitespace: bold,
+    });
+
+    const props = element.props as {
+      whitespace?: boolean;
+      components: (tag?: string) => unknown;
+    };
+    // The real id still reaches `say.call` and the tag named after it does not
+    // displace it, while a tag named `whitespace` is a value like any other and
+    // the flag itself still reaches the renderer.
+    expect(call).toHaveBeenCalledWith({ id: 'greet', whitespace: bold });
+    expect(props.whitespace).toBe(false);
+    expect(typeof props.components('id')).toBe('function');
+    expect(typeof props.components('whitespace')).toBe('function');
+  });
+
   it('exposes Plural, Ordinal and Select macros that throw', () => {
     expect(() => Say.Plural({ _: 1, other: '#' })).toThrow("'Say.Plural' is a macro");
     expect(() => Say.Ordinal({ _: 1, other: '#' })).toThrow("'Say.Ordinal' is a macro");

packages/integration-react/src/runtime/index.ts

@@ -8,7 +8,7 @@ import {
 } from 'react';
 import type { Disallow, NumeralOptions, SelectOptions } from 'saykit';
 import { Renderer } from '~/components/renderer.js';
-import { type PropsWithJSXSafeKeys, resolveJsxSafePropKeys } from '~/types.js';
+import { type PropsWithJSXSafeKeys, resolveValuePropKeys } from '~/types.js';
 
 declare function GET_SAY(): import('saykit').ReadonlySay;
 
@@ -30,15 +30,17 @@ export function Say(props: { id: string; whitespace?: boolean; [match: string]:
     });
 
   const say = GET_SAY();
-  const { whitespace, ...rest } = props;
-  const descriptor = resolveJsxSafePropKeys(rest);
+  const { id, whitespace, ...rest } = props;
+  const values = resolveValuePropKeys(rest);
 
   return createElement(Renderer, {
-    html: say.call(descriptor),
+    // The id is kept out of `values` and merged in last, so a message free to
+    // name a tag `id` still cannot displace the message being looked up.
+    html: say.call({ ...values, id }),
     whitespace,
     components(tag?: string) {
-      if (tag && tag in descriptor && isValidElement(descriptor[tag])) {
-        const element = descriptor[tag]! as ReactElement;
+      if (tag && tag in values && isValidElement(values[tag])) {
+        const element = values[tag]! as ReactElement;
         return (props) => cloneElement(element, { ...(element.props as object), ...props });
       } else {
         return tag;