Skip to content

Fix Parser.name() to treat '}' as a terminator for shorthand properties - #422

Open
jasonmobley wants to merge 2 commits into
knockout:mainfrom
jasonmobley:parse-es6-shorthand
Open

Fix Parser.name() to treat '}' as a terminator for shorthand properties#422
jasonmobley wants to merge 2 commits into
knockout:mainfrom
jasonmobley:parse-es6-shorthand

Conversation

@jasonmobley

@jasonmobley jasonmobley commented Aug 11, 2026

Copy link
Copy Markdown

An ES6 shorthand object property immediately followed by '}' (e.g. {a}) failed to parse because the terminator set in name() didn't include '}', causing the scanner to consume it and run past the end of the object literal. Add regression tests covering the bare-brace case plus its variants in function-call args and array elements.

Fixes #421

Summary by CodeRabbit

  • Bug Fixes

    • Improved parsing of unquoted binding names when followed by a closing brace.
    • Added support for quoted binding names containing spaces, commas, pipes, colons, and braces.
    • Improved handling of ES6 shorthand properties across objects, function arguments, and array elements.
  • Tests

    • Added coverage for shorthand properties, mixed declarations, trailing commas, and related parsing scenarios.

An ES6 shorthand object property immediately followed by '}' (e.g. `{a}`)
failed to parse because the terminator set in name() didn't include '}',
causing the scanner to consume it and run past the end of the object
literal. Add regression tests covering the bare-brace case plus its
variants in function-call args and array elements.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: feebd82c-1208-4286-aec0-ef32cc047f60

📥 Commits

Reviewing files that changed from the base of the PR and between 205a452 and e5b6cfb.

📒 Files selected for processing (2)
  • packages/utils.parser/spec/parserBehaviors.ts
  • packages/utils.parser/src/Parser.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/utils.parser/src/Parser.ts

📝 Walkthrough

Walkthrough

The parser now treats } as a delimiter for unquoted binding names. Tests cover ES6 shorthand object properties and quoted binding names containing terminator characters.

Changes

ES6 shorthand object property parsing

Layer / File(s) Summary
Support shorthand properties before closing braces
packages/utils.parser/src/Parser.ts, packages/utils.parser/spec/parserBehaviors.ts
Parser.name() stops unquoted names at }. Tests cover terminal, multiple, mixed, trailing-comma, function-argument, and array-element shorthand properties. Additional tests verify quoted names containing braces, spaces, commas, pipes, and colons.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the parser fix for treating '}' as a terminator for shorthand properties.
Linked Issues check ✅ Passed The parser change and regression tests address issue #421, including shorthand properties before '}' and supported variants.
Out of Scope Changes check ✅ Passed All changes are limited to the required parser behavior and related regression tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Caution

CodeRabbit couldn't update its existing comment. The review summary may be out of date.

Error details
putComment timed out

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/utils.parser/spec/parserBehaviors.ts (1)

137-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a whitespace regression case.

The objective requires shorthand properties with whitespace before } to remain supported. The added cases do not exercise input such as x: { a }.

Proposed test
+    it('parses a shorthand property with whitespace before }', function () {
+      const bindings = new Parser().parse('x: { a }', ctxStub({ a: 1 }))
+      assert.equal(bindings.x().a, 1)
+    })
🤖 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 `@packages/utils.parser/spec/parserBehaviors.ts` around lines 137 - 171, Add a
regression test in the shorthand ES6 object properties suite covering whitespace
around the shorthand property, such as parsing `x: { a }` with `ctxStub({ a: 1
})`; assert that the resulting object contains key `a` with value `1`.
🤖 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/utils.parser/src/Parser.ts`:
- Line 136: Update the name-termination condition in Parser’s name parsing logic
so the `}` delimiter applies only when `enclosedBy` is unset, preserving `}`
inside quoted names. Add a regression test covering parsing a binding such as
`"a}b": 1` and verify the complete quoted name is returned successfully.

---

Nitpick comments:
In `@packages/utils.parser/spec/parserBehaviors.ts`:
- Around line 137-171: Add a regression test in the shorthand ES6 object
properties suite covering whitespace around the shorthand property, such as
parsing `x: { a }` with `ctxStub({ a: 1 })`; assert that the resulting object
contains key `a` with value `1`.
🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 906a6700-ab26-4824-9056-e3dbc6ef613e

📥 Commits

Reviewing files that changed from the base of the PR and between d1ea9e8 and 205a452.

📒 Files selected for processing (2)
  • packages/utils.parser/spec/parserBehaviors.ts
  • packages/utils.parser/src/Parser.ts

Comment thread packages/utils.parser/src/Parser.ts Outdated
The terminator characters (':', whitespace, ',', '|', '}') only mark the
end of an unquoted name. Without the !enclosedBy guard, any of these
characters occurring inside a quoted name (e.g. "a}b", "a b", "a,b")
would end the name early, since the check ran unconditionally instead of
only when not inside a quoted string. This predates the prior '}'
addition -- space/comma/pipe already had the same defect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parsing fails when binding expression contains ES6 property shorthand immediately before closing brace

1 participant