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

stream: make null an invalid chunk to write in object mode #6170

Closed
wants to merge 1 commit into from

Conversation

@calvinmetcalf
Copy link
Contributor

@calvinmetcalf calvinmetcalf commented Apr 12, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added (current documentation implies this behavior)
  • the commit message follows commit guidelines
Affected core subsystem(s)
Description of change

as discussed by @nodejs/streams

this harmonizes behavior between readable, writable, and transform
streams so that they all handle nulls in object mode the same way by
considering them invalid chunks.

@calvinmetcalf calvinmetcalf force-pushed the calvinmetcalf:fail-write-null branch Apr 12, 2016
@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Apr 12, 2016

@calvinmetcalf mind linking to the discussion for posterity?

@calvinmetcalf
Copy link
Contributor Author

@calvinmetcalf calvinmetcalf commented Apr 12, 2016

Yeah which reminds me I have to figure out how to post that

On Tue, Apr 12, 2016, 4:23 PM Jeremiah Senkpiel notifications@github.com
wrote:

@calvinmetcalf https://github.com/calvinmetcalf mind linking to the
discussion for posterity?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#6170 (comment)

@jasnell
jasnell reviewed Apr 13, 2016
View changes
test/parallel/test-stream-writable-null.js Outdated

var stream = require('stream');
var util = require('util');

This comment has been minimized.

@jasnell

jasnell Apr 13, 2016
Member

The requires can use const :-)

@jasnell
jasnell reviewed Apr 13, 2016
View changes
test/parallel/test-stream-writable-null.js Outdated
assert.throws(() => {
var m = new MyWritable({objectMode: true});
m.write(null, (err) => assert.ok(err));
});

This comment has been minimized.

@jasnell

jasnell Apr 13, 2016
Member

nit: check the error type or error message here?

@jasnell
jasnell reviewed Apr 13, 2016
View changes
test/parallel/test-stream-writable-null.js Outdated
var m = new MyWritable({objectMode: true}).on('error', (e) => {
assert.ok(e);
});
m.write(null, function(err) {

This comment has been minimized.

@jasnell

jasnell Apr 13, 2016
Member

nit: why not an arrow function here also?

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 13, 2016

Few nits, but otherwise LGTM

@@ -177,13 +177,17 @@ function writeAfterEnd(stream, cb) {
// how many bytes or characters.

This comment has been minimized.

@mcollina

mcollina Apr 13, 2016
Member

I would add an explanation on the validation rules on before this function.

This comment has been minimized.

@calvinmetcalf

calvinmetcalf Apr 13, 2016
Author Contributor

this somewhat implies I 100% understand the current validation rules 😕

@mcollina
Copy link
Member

@mcollina mcollina commented Apr 13, 2016

LGTM.

Even though it's a major change, I will run it through CITGM, we should have no breakage.

You might also give a spin to the benchmarks, just to check if there any regression (I had some unexpected surprises in the past).

@calvinmetcalf calvinmetcalf force-pushed the calvinmetcalf:fail-write-null branch Apr 13, 2016
@calvinmetcalf
Copy link
Contributor Author

@calvinmetcalf calvinmetcalf commented Apr 13, 2016

ok fixed the nits and also reordered the checks to make them more clear and added the note

@jasnell
jasnell reviewed Apr 13, 2016
View changes
lib/_stream_writable.js Outdated
var er = false;
// always throw error if a null is written
// if we are not in object mode then throw
// if it is not a buffer, string, or undefined

This comment has been minimized.

@jasnell

jasnell Apr 13, 2016
Member

very minor nit: capitalize Always, add punctuation at the end.

@mcollina
Copy link
Member

@mcollina mcollina commented Apr 13, 2016

LGTM again :)

this harmonizes behavior between readable, writable, and transform
streams so that they all handle nulls in object mode the same way by
considering them invalid chunks.
@calvinmetcalf calvinmetcalf force-pushed the calvinmetcalf:fail-write-null branch to 401ff75 Apr 13, 2016
@jasnell
Copy link
Member

@jasnell jasnell commented Apr 18, 2016

@nodejs/streams ... is this ready to go?

@mcollina
Copy link
Member

@mcollina mcollina commented Apr 19, 2016

All good for me.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 20, 2016

I plan to land this one today but I'm asking @thealphanerd to do a quick smoketest after landing 75487f0

jasnell added a commit that referenced this pull request Apr 20, 2016
this harmonizes behavior between readable, writable, and transform
streams so that they all handle nulls in object mode the same way by
considering them invalid chunks.

PR-URL: #6170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member

@jasnell jasnell commented Apr 20, 2016

Landed in e7c077c

@jasnell jasnell closed this Apr 20, 2016
joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
this harmonizes behavior between readable, writable, and transform
streams so that they all handle nulls in object mode the same way by
considering them invalid chunks.

PR-URL: nodejs#6170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Apr 26, 2016
this harmonizes behavior between readable, writable, and transform
streams so that they all handle nulls in object mode the same way by
considering them invalid chunks.

PR-URL: #6170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants