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

Import Node 10.9.0. Redo the fix for util.inspect.custom #357

Merged
merged 3 commits into from
Aug 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ npm install --save readable-stream

This package is a mirror of the streams implementations in Node.js.

Full documentation may be found on the [Node.js website](https://nodejs.org/dist/v10.8.0/docs/api/stream.html).
Full documentation may be found on the [Node.js website](https://nodejs.org/dist/v10.9.0/docs/api/stream.html).

If you want to guarantee a stable streams base, regardless of what version of
Node you, or the users of your libraries are using, use **readable-stream** *only* and avoid the *"stream"* module in Node-core, for background see [this blogpost](http://r.va.gg/2014/06/why-i-dont-use-nodes-core-stream-module.html).
Expand Down
23 changes: 5 additions & 18 deletions build/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,6 @@ function CorkedRequest(state) {
/Object\.getPrototypeOf\((chunk)\) !== Buffer\.prototype/g
, '!Buffer.isBuffer($1)'
]
, fixCopyBuffer = [
/Buffer\.prototype\.copy\.call\(src, target, offset\);/
, 'src.copy(target, offset);'
]
, errorsOneLevel = [
/internal\/errors/
, '../errors'
Expand Down Expand Up @@ -280,24 +276,15 @@ module.exports['_stream_writable.js'] = [

module.exports['internal/streams/buffer_list.js'] = [
[
/(?:var|const) (?:{ )Buffer(?: }) = require\('buffer'\)(?:\.Buffer)?;/,
/const \{ inspect \} = require\('util'\);/,
`
const Buffer = require('buffer').Buffer
const util = require('util')
const { inspect } = require('util')
const custom = inspect && inspect.custom || 'inspect'
Copy link
Member

Choose a reason for hiding this comment

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

Won't this browserify all of util? I wrote https://github.com/mafintosh/inspect-custom-symbol to get around that (util is pretty big!)

Copy link
Member Author

Choose a reason for hiding this comment

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

@mafintosh that module uses the symbol instead of 'inspect' as a key. See https://github.com/mafintosh/inspect-custom-symbol/blob/master/index.js#L4. @BridgeAR should we use the symbol or the property here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also see #356 to remove the use of util.

Copy link
Member

Choose a reason for hiding this comment

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

It should be 'inspect'. Otherwise old Node.js versions won't work and a symbol has no other effect on browserify in this case than the string.

I don't use browserify often enough to know exactly what it would pull in but it should only be the parts that are used and in this case it is only the inspection property. So that should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should land this as is and deal with removing util in #356. What do you think? May I get a couple of LGTM?

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR it pulls in the entire util source. that's why people use the inherits module instead of util.inherits on npm also :)

@mcollina Ya, solving it elsewhere is fine by me

`
]
, fixCopyBuffer
, [
/$/,
`

if (util && util.inspect && util.inspect.custom) {
module.exports.prototype[util.inspect.custom] = function () {
const obj = util.inspect({ length: this.length });
return \`\${this.constructor.name} \${obj}\`;
}
}
`
/inspect.custom/g,
'custom'
]

]
Expand Down
3 changes: 2 additions & 1 deletion lib/_stream_readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ Readable.prototype.read = function (n) {
// if we're doing read(0) to trigger a readable event, but we
// already have a bunch of data in the buffer, then just trigger
// the 'readable' event and move on.
if (n === 0 && state.needReadable && (state.length >= state.highWaterMark || state.ended)) {
if (n === 0 && state.needReadable && ((state.highWaterMark !== 0 ? state.length >= state.highWaterMark : state.length > 0) || state.ended)) {
debug('read: emitReadable', state.length, state.ended);
if (state.length === 0 && state.ended) endReadable(this);else emitReadable(this);
return null;
Expand Down Expand Up @@ -768,6 +768,7 @@ Readable.prototype.on = function (ev, fn) {
if (!state.endEmitted && !state.readableListening) {
state.readableListening = state.needReadable = true;
state.emittedReadable = false;
debug('on readable', state.length, state.reading);
if (state.length) {
emitReadable(this);
} else if (!state.reading) {
Expand Down
23 changes: 9 additions & 14 deletions lib/internal/streams/buffer_list.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

var Buffer = require('buffer').Buffer;
var util = require('util');
var _require = require('buffer'),
Buffer = _require.Buffer;

var _require = require('util'),
inspect = _require.inspect;
var _require2 = require('util'),
inspect = _require2.inspect;

var custom = inspect && custom || 'inspect';

function copyBuffer(src, target, offset) {
src.copy(target, offset);
Buffer.prototype.copy.call(src, target, offset);
}

module.exports = function () {
Expand Down Expand Up @@ -152,17 +154,10 @@ module.exports = function () {
return ret;
};

BufferList.prototype[inspect.custom] = function () {
BufferList.prototype[custom] = function () {
var obj = inspect({ length: this.length });
return this.constructor.name + ' ' + obj;
};

return BufferList;
}();

if (util && util.inspect && util.inspect.custom) {
module.exports.prototype[util.inspect.custom] = function () {
var obj = util.inspect({ length: this.length });
return this.constructor.name + ' ' + obj;
};
}
}();
6 changes: 0 additions & 6 deletions test/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,6 @@ if (!global.console.info) {
var test = require('tape');
var util = require('util');

// TODO: add replacements instead
if (!util.inspect) {
util.inspect = function () {};
util.inspect.custom = 'custom';
}

// TODO: add replacements instead
global.process = {
env: {},
Expand Down
24 changes: 9 additions & 15 deletions test/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,6 @@ Tests whether `name`, `expected`, and `code` are part of a raised warning. If
an expected warning does not have a code then `common.noWarnCode` can be used
to indicate this.

### fileExists(pathname)
* pathname [<string>]
* return [<boolean>]

Checks if `pathname` exists

### getArrayBufferViews(buf)
* `buf` [<Buffer>]
* return [<ArrayBufferView[]>]
Expand Down Expand Up @@ -582,7 +576,7 @@ one listed below. (`heap.validateSnapshotNodes(...)` is a shortcut for

Create a heap dump and an embedder graph copy and validate occurrences.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
validateSnapshotNodes('TLSWRAP', [
{
Expand All @@ -600,7 +594,7 @@ validateSnapshotNodes('TLSWRAP', [
The http2.js module provides a handful of utilities for creating mock HTTP/2
frames for testing of HTTP/2 endpoints

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-unused-vars, node-core/required-modules -->
```js
const http2 = require('../common/http2');
```
Expand All @@ -610,7 +604,7 @@ const http2 = require('../common/http2');
The `http2.Frame` is a base class that creates a `Buffer` containing a
serialized HTTP/2 frame header.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
// length is a 24-bit unsigned integer
// type is an 8-bit unsigned integer identifying the frame type
Expand All @@ -629,7 +623,7 @@ The serialized `Buffer` may be retrieved using the `frame.data` property.
The `http2.DataFrame` is a subclass of `http2.Frame` that serializes a `DATA`
frame.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
// id is the 32-bit stream identifier
// payload is a Buffer containing the DATA payload
Expand All @@ -646,7 +640,7 @@ socket.write(frame.data);
The `http2.HeadersFrame` is a subclass of `http2.Frame` that serializes a
`HEADERS` frame.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
// id is the 32-bit stream identifier
// payload is a Buffer containing the HEADERS payload (see either
Expand All @@ -664,7 +658,7 @@ socket.write(frame.data);
The `http2.SettingsFrame` is a subclass of `http2.Frame` that serializes an
empty `SETTINGS` frame.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
// ack is a boolean indicating whether or not to set the ACK flag.
const frame = new http2.SettingsFrame(ack);
Expand All @@ -677,7 +671,7 @@ socket.write(frame.data);
Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2
request headers to be used as the payload of a `http2.HeadersFrame`.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
const frame = new http2.HeadersFrame(1, http2.kFakeRequestHeaders, 0, true);

Expand All @@ -689,7 +683,7 @@ socket.write(frame.data);
Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2
response headers to be used as the payload a `http2.HeadersFrame`.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
const frame = new http2.HeadersFrame(1, http2.kFakeResponseHeaders, 0, true);

Expand All @@ -701,7 +695,7 @@ socket.write(frame.data);
Set to a `Buffer` containing the preamble bytes an HTTP/2 client must send
upon initial establishment of a connection.

<!-- eslint-disable no-undef, no-unused-vars, node-core/required-modules, strict -->
<!-- eslint-disable no-undef, node-core/required-modules -->
```js
socket.write(http2.kClientMagic);
```
Expand Down
5 changes: 4 additions & 1 deletion test/common/benchmark.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ function runBenchmark(name, args, env) {

var mergedEnv = Object.assign({}, process.env, env);

var child = fork(runjs, argv, { env: mergedEnv, stdio: 'pipe' });
var child = fork(runjs, argv, {
env: mergedEnv,
stdio: ['inherit', 'pipe', 'inherit', 'ipc']
});
child.stdout.setEncoding('utf8');

var stdout = '';
Expand Down
14 changes: 3 additions & 11 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -518,17 +518,8 @@ exports.hasMultiLocalhost = function hasMultiLocalhost() {
return ret === 0;
};

exports.fileExists = function (pathname) {
try {
fs.accessSync(pathname);
return true;
} catch (err) {
return false;
}
};

exports.skipIfEslintMissing = function () {
if (!exports.fileExists(path.join(__dirname, '..', '..', 'tools', 'node_modules', 'eslint'))) {
if (!fs.existsSync(path.join(__dirname, '..', '..', 'tools', 'node_modules', 'eslint'))) {
exports.skip('missing ESLint');
}
};
Expand Down Expand Up @@ -768,7 +759,8 @@ exports.expectsError = function expectsError(fn, settings, exact) {
assert.fail('Expected one argument, got ' + util.inspect(arguments));
}
var descriptor = Object.getOwnPropertyDescriptor(error, 'message');
assert.strictEqual(descriptor.enumerable, false, 'The error message should be non-enumerable');
// The error message should be non-enumerable
assert.strictEqual(descriptor.enumerable, false);

var innerSettings = settings;
if ('type' in settings) {
Expand Down
4 changes: 0 additions & 4 deletions test/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import common from './index.js';

const {
PORT,
isMainThread,
isWindows,
isWOW64,
Expand Down Expand Up @@ -38,7 +37,6 @@ const {
mustCallAtLeast,
mustCallAsync,
hasMultiLocalhost,
fileExists,
skipIfEslintMissing,
canCreateSymLink,
getCallSite,
Expand Down Expand Up @@ -67,7 +65,6 @@ const {
} = common;

export {
PORT,
isMainThread,
isWindows,
isWOW64,
Expand Down Expand Up @@ -98,7 +95,6 @@ export {
mustCallAtLeast,
mustCallAsync,
hasMultiLocalhost,
fileExists,
skipIfEslintMissing,
canCreateSymLink,
getCallSite,
Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-stream-readable-hwm-0.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

/*<replacement>*/
var bufferShim = require('safe-buffer').Buffer;
/*</replacement>*/

var common = require('../common');

// This test ensures that Readable stream will call _read() for streams
// with highWaterMark === 0 upon .read(0) instead of just trying to
// emit 'readable' event.

var assert = require('assert/');

var _require = require('../../'),
Readable = _require.Readable;

var r = new Readable({
// must be called only once upon setting 'readable' listener
read: common.mustCall(),
highWaterMark: 0
});

var pushedNull = false;
// this will trigger read(0) but must only be called after push(null)
// because the we haven't pushed any data
r.on('readable', common.mustCall(function () {
assert.strictEqual(r.read(), null);
assert.strictEqual(pushedNull, true);
}));
r.on('end', common.mustCall());
process.nextTick(function () {
assert.strictEqual(r.read(), null);
pushedNull = true;
r.push(null);
});
;require('tap').pass('sync run');var _list = process.listeners('uncaughtException');process.removeAllListeners('uncaughtException');_list.pop();_list.forEach(function (e) {
return process.on('uncaughtException', e);
});
30 changes: 20 additions & 10 deletions test/parallel/test-stream-transform-final.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,46 +63,56 @@ The order things are called
var t = new stream.Transform({
objectMode: true,
transform: common.mustCall(function (chunk, _, next) {
assert.strictEqual(++state, chunk, 'transformCallback part 1');
// transformCallback part 1
assert.strictEqual(++state, chunk);
this.push(state);
assert.strictEqual(++state, chunk + 2, 'transformCallback part 2');
// transformCallback part 2
assert.strictEqual(++state, chunk + 2);
process.nextTick(next);
}, 3),
final: common.mustCall(function (done) {
state++;
assert.strictEqual(state, 10, 'finalCallback part 1');
// finalCallback part 1
assert.strictEqual(state, 10);
setTimeout(function () {
state++;
assert.strictEqual(state, 11, 'finalCallback part 2');
// finalCallback part 2
assert.strictEqual(state, 11);
done();
}, 100);
}, 1),
flush: common.mustCall(function (done) {
state++;
assert.strictEqual(state, 12, 'flushCallback part 1');
// flushCallback part 1
assert.strictEqual(state, 12);
process.nextTick(function () {
state++;
assert.strictEqual(state, 15, 'flushCallback part 2');
// flushCallback part 2
assert.strictEqual(state, 15);
done();
});
}, 1)
});
t.on('finish', common.mustCall(function () {
state++;
assert.strictEqual(state, 13, 'finishListener');
// finishListener
assert.strictEqual(state, 13);
}, 1));
t.on('end', common.mustCall(function () {
state++;
assert.strictEqual(state, 16, 'end event');
// end event
assert.strictEqual(state, 16);
}, 1));
t.on('data', common.mustCall(function (d) {
assert.strictEqual(++state, d + 1, 'dataListener');
// dataListener
assert.strictEqual(++state, d + 1);
}, 3));
t.write(1);
t.write(4);
t.end(7, common.mustCall(function () {
state++;
assert.strictEqual(state, 14, 'endMethodCallback');
// endMethodCallback
assert.strictEqual(state, 14);
}, 1));
;require('tap').pass('sync run');var _list = process.listeners('uncaughtException');process.removeAllListeners('uncaughtException');_list.pop();_list.forEach(function (e) {
return process.on('uncaughtException', e);
Expand Down
Loading