Skip to content

docs(json): clarify suitability for NDJSON - #7205

Open
tredondo wants to merge 1 commit into
denoland:mainfrom
tredondo:main
Open

docs(json): clarify suitability for NDJSON#7205
tredondo wants to merge 1 commit into
denoland:mainfrom
tredondo:main

Conversation

@tredondo

@tredondo tredondo commented Jul 2, 2026

Copy link
Copy Markdown

Refrained from recommending any particular alternative since I haven't found one with Deno-level DX.

@CLAassistant

CLAassistant commented Jul 2, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.84%. Comparing base (0e5b1f2) to head (b20efc0).

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7205    +/-   ##
========================================
  Coverage   94.83%   94.84%            
========================================
  Files         617      617            
  Lines       51674    51422   -252     
  Branches     9350     9300    -50     
========================================
- Hits        49007    48769   -238     
+ Misses       2121     2112     -9     
+ Partials      546      541     -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — the second half of this is a genuinely useful clarification and I'd like to land it. Neither JsonParseStream nor ConcatenatedJsonParseStream parses incrementally inside an array, and people do arrive expecting otherwise, so calling that out at module level is the right instinct.

Two corrections needed on the rest of the wording, both inline. The short version: one of the new sentences is true for one of the two parse streams but not the other, and the format list ended up narrower than what the per-symbol docs already promise.

Comment thread json/mod.ts
* Utilities for parsing streaming JSON data.
* Utilities for parsing streaming JSON data (newline-delimited
* JSON Lines / NDJSON, or concatenated JSON). Each chunk must
* be a complete JSON value. Not suited for streaming elements

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Each chunk must be a complete JSON value" holds for JsonParseStream, but not for ConcatenatedJsonParseStream, which explicitly buffers across chunk boundaries. concatenated_json_parse_stream_test.ts:231-235 asserts that ["{", '\"foo\": \"bar\"}'] parses fine — a chunk that is not a complete JSON value.

As a module-level statement it therefore contradicts the documented behaviour of one of the module's own exports. It's also the reason JsonParseStream's docs pair it with TextLineStream while the concatenated one doesn't need to.

Scoping it fixes the whole problem — something like "JsonParseStream requires each chunk to be a complete JSON value; ConcatenatedJsonParseStream buffers across chunk boundaries."

Comment thread json/mod.ts

/**
* Utilities for parsing streaming JSON data.
* Utilities for parsing streaming JSON data (newline-delimited

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The format list drops JSON Text Sequences (RFC 7464), which both parse_stream.ts:16 and stringify_stream.ts:24 document as supported. Since the point of this PR is to make the summary more informative, narrowing it below what the symbol docs already claim works against that.

Small wording nit while you're here: "newline-delimited JSON Lines / NDJSON" says newline-delimited twice — "JSON Lines (NDJSON)" covers it.

Also worth noting the module exports JsonStringifyStream and canonicalize too, so "Utilities for parsing streaming JSON data" undersells it — not introduced by this PR, but you're right here if you feel like fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants