Skip to content

streams: refactor getHighWaterMark in state.js #20415

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.

Sign up for GitHub

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

Closed
danbev wants to merge 3 commits into from
Closed

streams: refactor getHighWaterMark in state.js #20415

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d49e5b
streams: refactor getHighWaterMark in state.js
danbev Apr 30, 2018
08bcceb
squash: remove options prefix
danbev May 2, 2018
66cfaa5
squash: update hwm check
danbev May 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions lib/internal/streams/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,19 @@

const { ERR_INVALID_OPT_VALUE } = require('internal/errors').codes;

function highWaterMarkFrom(options, isDuplex, duplexKey) {
return options.highWaterMark != null ? options.highWaterMark :
isDuplex ? options[duplexKey] : null;
}

function getHighWaterMark(state, options, duplexKey, isDuplex) {
let hwm = options.highWaterMark;
const hwm = highWaterMarkFrom(options, isDuplex, duplexKey);
if (hwm != null) {
if (typeof hwm !== 'number' || !(hwm >= 0))
throw new ERR_INVALID_OPT_VALUE('highWaterMark', hwm);
return Math.floor(hwm);
} else if (isDuplex) {
hwm = options[duplexKey];
if (hwm != null) {
if (typeof hwm !== 'number' || !(hwm >= 0))
throw new ERR_INVALID_OPT_VALUE(duplexKey, hwm);
return Math.floor(hwm);
if (typeof hwm !== 'number' || !(hwm >= 0)) {

@apapirovski apapirovski Apr 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also change hwm >= 0 to hwm < 0? That negation is just confusing, I think.

@lpinca lpinca Apr 30, 2018

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.

Agreed.

@BridgeAR BridgeAR Apr 30, 2018

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.

That has a different meaning though because it does not catch NaN anymore.

@apapirovski apapirovski Apr 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good point. In that case maybe we should be checking for that explicitly? The fact that both me and @lpinca missed it means it is likely to get missed again in the future and then no one might correct it.

@BridgeAR BridgeAR Apr 30, 2018

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.

Well, hopefully we have tests for it ;-) Besides that, I guess it would be good to add a comment next to it.

@lpinca lpinca Apr 30, 2018 •
edited
Loading

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.

It seems we don't as CI is green for this :)

It wasn't changed yet, sorry.

@apapirovski apapirovski Apr 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we have tests actually. @danbev hasn't made the change yet. Anyway, I'm fine with adding a comment. Just not a big fan of non-obvious checks like this.

@lpinca lpinca Apr 30, 2018 •
edited
Loading

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.

Indeed, it would be semver-major but using Number.isInteger() seems the ideal solution.

@danbev danbev May 1, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry about the delay on this. I'll take a closer look tomorrow (public holiday here today)

const name = isDuplex ? duplexKey : 'highWaterMark';
throw new ERR_INVALID_OPT_VALUE(`options.${name}`, hwm);

@apapirovski apapirovski Apr 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think the options. bit is necessary? IMO it's more readable without.

@danbev danbev May 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only reason for making this change was to be consistent with #20284 but I'm happy to remove this.

@apapirovski apapirovski May 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The error constructors are different though. This one is more specific since it's not ERR_INVALID_ARG_TYPE but rather ERR_INVALID_OPT_VALUE. Meaning it already communicates that it's within the options object.

}
return Math.floor(hwm);
}

// Default value
Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-stream-transform-split-highwatermark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,17 @@ testTransform(0, 0, {
}, {
type: TypeError,
code: 'ERR_INVALID_OPT_VALUE',
message: 'The value "NaN" is invalid for option "readableHighWaterMark"'
message: 'The value "NaN" is invalid for option ' +
'"options.readableHighWaterMark"'
});

common.expectsError(() => {
new Transform({ writableHighWaterMark: NaN });
}, {
type: TypeError,
code: 'ERR_INVALID_OPT_VALUE',
message: 'The value "NaN" is invalid for option "writableHighWaterMark"'
message: 'The value "NaN" is invalid for option ' +
'"options.writableHighWaterMark"'
});
}

Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-streams-highwatermark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ for (const invalidHwm of [true, false, '5', {}, -5, NaN]) {
}, {
type: TypeError,
code: 'ERR_INVALID_OPT_VALUE',
message: `The value "${invalidHwm}" is invalid for option "highWaterMark"`
message: `The value "${invalidHwm}" is invalid for option ` +
'"options.highWaterMark"'
});
}
}