Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(encoding): add json/stream.ts #2231

Merged
merged 21 commits into from
Jun 20, 2022
Merged

feat(encoding): add json/stream.ts #2231

merged 21 commits into from
Jun 20, 2022

Conversation

ayame113
Copy link
Contributor

@ayame113 ayame113 commented May 15, 2022

close #2208

Implementations have been ported from https://github.com/ayame113/jsonlines/tree/9e969323a0b09d573a4a35f0d466866bb62e1a67.

#2227 needs to be merged first. The CI should turn green when #2227 is merged into main.

@ayame113 ayame113 marked this pull request as ready for review May 16, 2022 01:41
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@ayame113 LGTM. Great!

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, too!

@crowlKats crowlKats self-requested a review May 24, 2022 11:09
Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM, besides two minor nitpicks

encoding/json/_stringify_test.ts Outdated Show resolved Hide resolved
encoding/json/_stringify_test.ts Outdated Show resolved Hide resolved
@lucacasonato
Copy link
Member

TBH, I think these combinators are too specific. A JSONLinesParseStream is the same as a LineStream + JSONParseStream. For stream combinators we should provide low level primitives that users can orchestrate together themselves instead of providing one trick ponys.

I think JSONParseStream (that just parses each chunk as is), and JSONStringifyStream (which stringifies each chunk as is), are much better combinators.

@lucacasonato
Copy link
Member

For concatenation, I think a stream combinator that has an API in the form JoinStream<T>((i: number) => T | Promise<T>) that calls the callback for each chunk, and enqueue the result after each chunk. Please bikeshed the name though, as JoinStream is pretty terrible.

@nayeemrmn
Copy link
Contributor

IMO we should change this and the jsonc module to use Json casing as we do with fromFileUrl()

@kt3k
Copy link
Member

kt3k commented May 24, 2022

@lucacasonato

TBH, I think these combinators are too specific. A JSONLinesParseStream is the same as a LineStream + JSONParseStream. For stream combinators we should provide low level primitives that users can orchestrate together themselves instead of providing one trick ponys.

This suggestion makes sense to me. You mean the users should be able to do:

const readable = body!
  .pipeThrough(new TextDecoderStream())
  .pipeThrough(new TextDelimiterStream(separator))
  .pipeThrough(new JSONParseStream());

instead of:

const readable = body!
  .pipeThrough(new TextDecoderStream())
  .pipeThrough(new JSONLinesParseStream()); 

I also also agree with reducing JSONLinesStringifyStream and ConcatenatedJSONStringifyStream to single JSONStringifyStream. These two look almost the same.

For concatenation, I think a stream combinator that has an API in the form JoinStream((i: number) => T | Promise) that calls the callback for each chunk, and enqueue the result after each chunk. Please bikeshed the name though, as JoinStream is pretty terrible.

Is this suggestion about ConcatenatedJSONParseStream? I don't see how JoinStream covers what ConcatenatedJSONParseStream does. In my view ConcatenatedJSONParseStream is not trivial thing and can't be reduced to lower level primitives.

@kt3k
Copy link
Member

kt3k commented May 24, 2022

IMO we should change this and the jsonc module to use Json casing as we do with fromFileUrl()

but we already have JSON. Json casing feels strange to me..

@kt3k
Copy link
Member

kt3k commented May 24, 2022

wait we also have URL..

@ayame113
Copy link
Contributor Author

Maybe the usage will be like this?

// parses JSON lines, NDJSON and JSON Text Sequences
const readable = body!
  .pipeThrough(new TextDecoderStream())
  .pipeThrough(new TextDelimiterStream(separator)) // or TextLineStream
  .pipeThrough(new JSONParseStream());

// parses Concatenated JSON
const readable = body!
  .pipeThrough(new TextDecoderStream())
  .pipeThrough(new ConcatenatedJSONParseStream());

// stringifies JSON lines, NDJSON, JSON Text Sequences and Concatenated JSON
readable
  .pipeThrough(new JSONStringifyStream({ prefix, suffix })) // default of prefix is "", default of suffix is "\n"
  .pipeThrough(new TextEncoderStream());

// or
// readable
//  .pipeThrough(new JSONStringifyStream())
//  .pipeThrough(new MappingStream((str) => `${prefix}${str}${suffix}`)) // The name is tentative
//  .pipeThrough(new TextEncoderStream());

I don't have time this week, so I think I'll be working on the weekend.

@kt3k
Copy link
Member

kt3k commented May 25, 2022

@ayame113 The interface looks cleaner to me. Let's switch to it

@ayame113 ayame113 marked this pull request as draft May 29, 2022 08:01
@ayame113 ayame113 marked this pull request as ready for review May 29, 2022 10:12
@ayame113
Copy link
Contributor Author

ayame113 commented May 29, 2022

The current interface looks like this:

  • new JSONParseStream() - Parse each chunk as JSON
  • new ConcatenatedJSONParseStream() - Parse Concatenated JSON
  • new JSONStringifyStream({prefix, suffix}) - Convert each chunk to a JSON string and add a prefix and suffix

What about JSON casing? Do I need to change it?

@bartlomieju
Copy link
Member

I'm also in favor of keeping JSON casing.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM again.

I'm in favor of JSON casing.

@kt3k kt3k merged commit 6d33b18 into denoland:main Jun 20, 2022
cjihrig pushed a commit to bartlomieju/deno_std that referenced this pull request Jun 28, 2022
@ayame113 ayame113 deleted the stream-json branch March 18, 2023 04:35
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.

Proposal: Web stream based jsonlines parser
6 participants