Name placeholders with a single-key object, and keep compiled values out of the descriptor's namespace - #68
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: bc4c100 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
WalkthroughThe PR adds named single-key placeholders, prefixes transformed interpolation values with underscores, resolves those keys at runtime, prevents ChangesNamed placeholder flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SourceMacro
participant Transformer
participant ReactRuntime
participant SayCall
participant Formatter
SourceMacro->>Transformer: interpolated values and named placeholder objects
Transformer->>ReactRuntime: generated descriptor with underscore-prefixed values
ReactRuntime->>SayCall: descriptor and component resolution
SayCall->>Formatter: values after one-underscore resolution
Formatter->>ReactRuntime: formatted message
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/integration/src/runtime.ts`:
- Around line 12-19: Update resolveDescriptorValues to iterate only over
descriptor’s own entries and construct the result with a prototype-safe approach
such as Object.fromEntries. Preserve the existing id exclusion and
underscore-remapping behavior, while ensuring a __proto__ descriptor key remains
a normal placeholder value rather than mutating the result prototype.
In `@website/content/core-concepts/messages.mdx`:
- Around line 84-88: Update the Callout text to remove the claim that
interpolating objects never changes meaning. State that inline single-key
objects now change from object-value formatting to placeholder naming, while
objects held in variables and unsupported forms such as multiple keys, spreads,
or computed keys retain their previous behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cebe025b-be47-46bf-8372-09647b629b05
📒 Files selected for processing (21)
.changeset/brave-moons-repeat.md.changeset/olive-hounds-invent.mdpackages/integration-react/src/runtime/index.test.tspackages/integration-react/src/runtime/index.tspackages/integration/src/runtime.test.tspackages/integration/src/runtime.tspackages/integration/src/types.tspackages/transform-js/src/generator.test.tspackages/transform-js/src/generator.tspackages/transform-js/src/index.test.tspackages/transform-js/src/parser.test.tspackages/transform-js/src/parser.tspackages/transform-jsx/src/index.test.tspackages/transform-jsx/src/parser.tswebsite/content/core-concepts/architecture.mdxwebsite/content/core-concepts/messages.mdxwebsite/content/core-concepts/runtime.mdxwebsite/content/guides/custom-transformer.mdxwebsite/content/integrations/react.mdxwebsite/content/integrations/vite.mdxwebsite/content/reference/api/saykit.mdx
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
wrong acc |
|
wait, can i use alt accs to bypass coderabbit 1 per hour |
…-names # Conflicts: # packages/integration-react/src/runtime/index.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/content/core-concepts/messages.mdx (1)
289-289: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the comma before “so”.
Use “what the element does, so you can name one” for correct grammar.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/content/core-concepts/messages.mdx` at line 289, Update the sentence in the messages documentation to remove the comma before “so,” using the phrasing “what the element does, so you can name one” while preserving the surrounding explanation and link.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@website/content/core-concepts/messages.mdx`:
- Line 289: Update the sentence in the messages documentation to remove the
comma before “so,” using the phrasing “what the element does, so you can name
one” while preserving the surrounding explanation and link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f150e00-00f3-4ee7-9274-49bf84d3b3cf
📒 Files selected for processing (2)
packages/integration-react/src/runtime/index.test.tswebsite/content/core-concepts/messages.mdx
Closes #67. Targets
main, with #66 merged in — this extends its underscore convention from JSX to plain JS.Two gaps in the plain-JS pipeline: placeholders that cannot be named, and a value named
idsilently breaking the message lookup.Naming a placeholder
An interpolated variable is already named after itself, so only the values that would otherwise be numbered need anything. Interpolate a single-key object and the key is the name:
Nothing to import, and nothing survives the build — the transform reads the key and compiles only the value. It works the same in
say.plural/ordinal/selectselectors and in<Say>interpolations:What is and is not claimed
Only an object written inline with exactly one non-computed key whose value is an expression. A variable holding an object, an object with two keys, a spread, a computed key, and a method are all left exactly as they were:
This is the reason for choosing the shape: interpolating an object stringifies to
[object Object]and rendering one in JSX throws, so there is no working code whose meaning changes. A function macro (ph('name', value)) was the alternative, and was rejected because it needs an import, is matched by name, and can be shadowed.The name must be a valid identifier — a letter or underscore followed by letters, digits, or underscores — matching the rule
say-taguses. An invalid one fails the build rather than producing an unusable ICU name. Naming is opt-in per placeholder, so anything left alone keeps its number.Recognition lives in one exported function,
unwrapPlaceholderin@saykit/transform-js/parser, which@saykit/transform-jsximports at all three sites that used to derive an identifier from an expression; its duplicate local helper is deleted rather than taught the same trick twice.A value named
idno longer breaks the lookupgenerateSayCallExpressionbuilt one flat object, so a value namedidemitted a duplicate key and won:descriptor.idbecame the runtime value of the variable, somessages[descriptor.id]missed and threwMessage for <value> is not a string— at runtime, in whichever locale happened to be active, with no build-time signal.Values are now compiled behind one underscore and
Say#callstrips exactly one back off, the same convention #66 introduced for JSX props:Keys written without an underscore are passed through untouched, so hand-written calls and custom transformers that emit
say.call({ id, ...values })keep working —examples/custom-formatterdoes exactly that and is the end-to-end proof.@saykit/reactstops stripping before it callssay.calland passes its props through prefixed, so there is exactly one strip in the pipeline and double-stripping is structurally impossible rather than a convention.The generator also dedupes repeated identifiers now, so
say`${a} and ${a}`emits one property instead of a duplicate key. Side benefit:_0is a real identifier, wheret.identifier('0')only worked because@babel/generatorprints the name raw.Types
say.plural(_: number, …)would reject{ items: cart.length }, so the three selector parameters widened tonumber | Named<number>(andstring | Named<string>) in both theSayclass andSay.Plural/Ordinal/Select.Named<T>is a new export fromsaykit. This is the one cost of the syntax: those parameters are looser than they were, since TypeScript cannot express "an object with exactly one key". The build-time check is what actually validates the shape.Breaking
{id}in a message is now fed by_idrather than resolving to the message's own id, which was never useful._foomust be written__foo.saykit,@saykit/transform-js,@saykit/transform-jsxand@saykit/reactmust be released together: a transform emitting_idagainst a runtime that does not strip it renders wrong output. Thesaykitpeer range in@saykit/reactis"*", so npm will not catch a mismatched pair — worth tightening separately.Verification
pnpm -r check,oxlintand the website build are clean.examples/vanillaandexamples/reactre-extract with no catalogue churn; the vanilla bundle now containscall({id:"pOt5xO",_0:Math.abs(...)})and still builds.examples/custom-formatter, which hand-writessay.call({ id, ...values })with unprefixed keys, still renders correctly.Duplicate names
A name belongs to a value, not a position, so the rule is the one #66 gave elements: same name, the expressions must match, or it is a build error.
No implicit/explicit distinction is needed: an implicit name is only ever a bare identifier, so two implicit collisions are the same variable by construction, and reusing
${name}stays free.${{ name: name }}beside${name}is also legal, since it is the same value written two ways.collectAssignedIdentifiersnow claims value names through the same comparison it already used for tags, andisEquivalentElementis replaced byisEquivalentPlaceholderin@saykit/transform-js/parser: elements compare on their opening element alone, everything else compares whole. Both transformers pass it —transform-jspassed no comparator at all before, which would have made the legal repeat an error.One consequence worth stating: a repeat is evaluated once, so
${{ total: getTotal() }}twice callsgetTotalonce. Documented rather than restricted, so the rule stays one sentence.Not in scope
Nothing outstanding.
Summary by CodeRabbit
{ who: name }) across message macros and JSX.