Skip to content

refactor: replace cjson with internal comment-stripping JSON parser#10662

Open
joehan wants to merge 1 commit into
mainfrom
joehan/remove-cjson
Open

refactor: replace cjson with internal comment-stripping JSON parser#10662
joehan wants to merge 1 commit into
mainfrom
joehan/remove-cjson

Conversation

@joehan

@joehan joehan commented Jun 15, 2026

Copy link
Copy Markdown
Member

Description

Removed the external third-party cjson dependency and replaced it with a robust custom comment-stripping parser in src/loadCJSON.ts. This reduces production dependency weight.

Scenarios Tested

  • Added a new complex.cjson test fixture with trailing, inline, and string-embedded comments.
  • Run npx mocha src/loadCJSON.spec.ts successfully.

Sample Commands

  • npx mocha src/loadCJSON.spec.ts

### Description
Removed the external third-party `cjson` dependency and replaced it with a robust custom comment-stripping parser in `src/loadCJSON.ts`. This reduces production dependency weight.

### Scenarios Tested
- Added a new `complex.cjson` test fixture with trailing, inline, and string-embedded comments.
- Run `npx mocha src/loadCJSON.spec.ts` successfully.

### Sample Commands
- `npx mocha src/loadCJSON.spec.ts`

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request replaces the external 'cjson' dependency with a custom comment-stripping implementation using regular expressions. A critical bug was identified in the new implementation where empty strings are incorrectly stripped from the JSON, leading to syntax errors during parsing. The feedback recommends fixing this by explicitly checking for undefined and suggests replacing comments with whitespace to preserve line numbers for improved error reporting.

Comment thread src/loadCJSON.ts
Comment on lines +19 to +25
const stripped = content.replace(
/("([^"\\]|\\.)*")|(\/\*[\s\S]*?\*\/|\/\/.*)/g,
(match, string) => {
if (string) return string;
return "";
},
);

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.

critical

Issue: Critical Bug with Empty Strings ""

The current implementation of the comment-stripping parser has a critical bug when parsing empty strings "" in JSON.

In the replacement callback:

(match, string) => {
  if (string) return string;
  return "";
}

The parameter string corresponds to the first capture group ("([^"\\]|\\.)*"). When the parser encounters an empty string "", the captured value is "". Since "" is a falsy value in JavaScript, if (string) evaluates to false, causing the function to return "" (which is treated as a comment removal).

This results in the empty string being completely stripped from the JSON, leading to invalid JSON and a SyntaxError during JSON.parse. For example, {"key": ""} becomes {"key": }, which fails to parse.

Suggestion: Preserve Line Numbers & Fix Empty Strings

We can fix this bug by explicitly checking if quotedString is not undefined.

Additionally, we can improve error reporting by replacing comments with spaces/newlines instead of an empty string "". This preserves the exact line and column numbers of the original file, making any JSON parsing error messages much more accurate and helpful.

    const stripped = content.replace(
      /("([^"\\]|\\.)*")|(\/\*[\s\S]*?\*\/|\/\/.*)/g,
      (match, quotedString) => {
        if (quotedString !== undefined) {
          return quotedString;
        }
        return match.replace(/[^\r\n]/g, " ");
      },
    );

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.

2 participants