Breaking: move to JSONStream. remove multibyte option #6
Conversation
DonutEspresso
force-pushed
the
jsonstream
branch
3 times, most recently
from
2c224e7
to
906a889
Compare
DonutEspresso
force-pushed
the
jsonstream
branch
from
906a889
to
a2d3616
Compare
test/index.js
Outdated
| @@ -188,79 +189,13 @@ describe('big-json', function() { | |||
| parseStream.on('error', function(err) { | |||
| assert.ok(err); | |||
| assert.equal(err.name, 'Error'); | |||
| assert.equal(err.message, 'Parser has expected a string value'); | |||
| assert.equal(err.message, 'Invalid JSON (Unexpected "\\n" ' + | |||
There was a problem hiding this comment.
Should we couple our tests to the error message? Is having an error with a message enough?
There was a problem hiding this comment.
Shortened to test just for 'Invalid JSON'. Coupling isn't great, but in this case I do want to verify that it's because of invalid content vs some other internal error.
test/index.js
Outdated
| return done(); | ||
| }); | ||
|
|
||
| readStream.pipe(parseStream); | ||
| }); | ||
|
|
||
|
|
||
| it('should handle multibyte keys and vals', function(done) { |
There was a problem hiding this comment.
Why are we dropping support for multibyte characters?
There was a problem hiding this comment.
Supported by default now (mentioned in OP :)).
There was a problem hiding this comment.
Shouldn't we keep the test to ensure future changes don't break multibyte?
lib/index.js
Outdated
| return callback(null, stringified); | ||
| } | ||
| return null; | ||
| return callback(null, stringified); |
There was a problem hiding this comment.
Do we need to wrap the callback in a strict once?
| @@ -161,24 +180,6 @@ describe('big-json', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| it('should throw on piping multiple source streams', function(done) { | |||
There was a problem hiding this comment.
Will this result in valid behaviour?
There was a problem hiding this comment.
In retrospect, technically up to the user - if they want to parse two separate sub objects and merge them into a single object using streams, this would allow them to do so.
This was disallowed before because the impl was done poorly. :)
DonutEspresso
force-pushed
the
jsonstream
branch
from
477e44d
to
a1ab36c
Compare
1 similar comment
2 similar comments
| return cb(); | ||
| }); | ||
| const parseStream = createParseStream(opts); | ||
| const sourceStream = intoStream(opts.body); |
There was a problem hiding this comment.
Conversion of in memory string to into readable chunks is going to help a lot - I suspect we were losing I/O here because pushing a giant string into a stream effectively becomes a giant JSON.parse
This moves this module to JSONStream.
The granularity provided by json-stream is nice but was causing parse operations to take much longer than expected. It looks like JSONStream by default using the
$*path separates on top level fields, which may be "good enough" for most big JSON cases.There might be something interesting around customizing the JSONStream path to target parts of the big POJO where known tokens are too large. Could be something for 2.x.
opts.multibyteis also no longer necessary since JSONStream's underlying jsonparse handles it already.