Skip to content

Propagate JSON parse errors - #326

Open
StantonMatt wants to merge 1 commit into
extrabacon:masterfrom
StantonMatt:fix-json-parse-error-propagation
Open

Propagate JSON parse errors#326
StantonMatt wants to merge 1 commit into
extrabacon:masterfrom
StantonMatt:fix-json-parse-error-propagation

Conversation

@StantonMatt

Copy link
Copy Markdown

Summary

  • catch exceptions from stdout parser functions before emitting message events
  • emit a parseError event and pass the first parse failure to .end() / PythonShell.run() so callers can handle invalid JSON output
  • keep user message listener exceptions outside the parser catch path so normal EventEmitter behavior is preserved

Fixes #253.

Validation

  • npm test -- --grep "parseError|JSON output"
  • npm test
  • git diff --check
  • review-fix-loop clean after addressing one reviewer finding

@kikiminyes

Copy link
Copy Markdown

I checked the core path locally.

  • npm run compileOnce passes.
  • I could not run the Python-dependent suite in this Windows environment because Python spawning fails before the tests run, so I validated the changed JS path with a mocked child process instead.
  • With invalid JSON on stdout, parseError is emitted, .end() receives PythonShellParseError, and PythonShell.run(..., { mode: "json" }) rejects with the same parse error and empty logs.

The implementation looks directionally correct to me. Keeping parse failures on a separate parseError event avoids Node's special unhandled error event behavior while still surfacing the failure through the completion path.

@PNHD PNHD left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The parser/emit ordering on this head is sound, and the focused/full npm suites plus repository Prettier check pass locally on an LF-safe checkout. One required piece is still missing before this is ready as a library API change: the PR exports PythonShellParseError and adds a public parseError event, but README's event documentation does not mention either. For direct PythonShell callers without an .end() callback, parseError is the parser-failure surface; leaving it undocumented can turn the old crash into a failure users do not know how to observe. Please document the new event/class, when it fires, and how .end() / PythonShell.run() receive the first parse failure. I have an overlapping #334 with a different API shape and will defer to maintainer preference rather than requiring either architecture here.

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.

Can't catch JSON.parse() error?

3 participants