From a1f4285ce87ffa3278818416998e4db9b45c4984 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 23 Nov 2015 14:58:18 -0500 Subject: [PATCH 1/9] util,src: allow lookup of hidden values This commit adds an internal util method that makes hidden values in the C++ layer visible in JS. PR-URL: https://github.com/nodejs/node/pull/3988 Reviewed-By: Trevor Norris Reviewed-By: Ben Noordhuis Conflicts: lib/internal/util.js --- lib/internal/util.js | 3 +++ src/node_util.cc | 19 +++++++++++++++++ test/parallel/test-util-internal.js | 32 +++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 test/parallel/test-util-internal.js diff --git a/lib/internal/util.js b/lib/internal/util.js index a31f22e6e9f186..5be073f18a2217 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -1,7 +1,10 @@ 'use strict'; +const binding = process.binding('util'); const prefix = '(node) '; +exports.getHiddenValue = binding.getHiddenValue; + // All the internal deprecations have to use this function only, as this will // prepend the prefix to the actual message. exports.deprecate = function(fn, msg) { diff --git a/src/node_util.cc b/src/node_util.cc index 19c3e3240a52af..a520b8d5f354ac 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -10,6 +10,7 @@ using v8::Context; using v8::FunctionCallbackInfo; using v8::Local; using v8::Object; +using v8::String; using v8::Value; static void IsMapIterator(const FunctionCallbackInfo& args) { @@ -28,6 +29,23 @@ static void IsPromise(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(args[0]->IsPromise()); } + +static void GetHiddenValue(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + if (!args[0]->IsObject()) + return env->ThrowTypeError("obj must be an object"); + + if (!args[1]->IsString()) + return env->ThrowTypeError("name must be a string"); + + Local obj = args[0].As(); + Local name = args[1].As(); + + args.GetReturnValue().Set(obj->GetHiddenValue(name)); +} + + void Initialize(Local target, Local unused, Local context) { @@ -35,6 +53,7 @@ void Initialize(Local target, env->SetMethod(target, "isMapIterator", IsMapIterator); env->SetMethod(target, "isSetIterator", IsSetIterator); env->SetMethod(target, "isPromise", IsPromise); + env->SetMethod(target, "getHiddenValue", GetHiddenValue); } } // namespace util diff --git a/test/parallel/test-util-internal.js b/test/parallel/test-util-internal.js new file mode 100644 index 00000000000000..9ab883ec8b6dcd --- /dev/null +++ b/test/parallel/test-util-internal.js @@ -0,0 +1,32 @@ +'use strict'; +// Flags: --expose_internals + +const common = require('../common'); +const assert = require('assert'); +const internalUtil = require('internal/util'); + +function getHiddenValue(obj, name) { + return function() { + internalUtil.getHiddenValue(obj, name); + }; +} + +assert.throws(getHiddenValue(), /obj must be an object/); +assert.throws(getHiddenValue(null, 'foo'), /obj must be an object/); +assert.throws(getHiddenValue(undefined, 'foo'), /obj must be an object/); +assert.throws(getHiddenValue('bar', 'foo'), /obj must be an object/); +assert.throws(getHiddenValue(85, 'foo'), /obj must be an object/); +assert.throws(getHiddenValue({}), /name must be a string/); +assert.throws(getHiddenValue({}, null), /name must be a string/); +assert.throws(getHiddenValue({}, []), /name must be a string/); +assert.deepEqual(internalUtil.getHiddenValue({}, 'foo'), undefined); + +let arrowMessage; + +try { + require('../fixtures/syntax/bad_syntax'); +} catch (err) { + arrowMessage = internalUtil.getHiddenValue(err, 'arrowMessage'); +} + +assert(/bad_syntax\.js:1/.test(arrowMessage)); From 75b638e0ac1039ab02dda63c83713190de3c2928 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 24 Nov 2015 19:52:35 -0500 Subject: [PATCH 2/9] util: add decorateErrorStack() This commit adds the decorateErrorStack() method. This function uses the internal util's getHiddenValue() method to extract arrow messages from error objects and attach them to the error's stack trace. PR-URL: https://github.com/nodejs/node/pull/4013 Reviewed-By: Ben Noordhuis --- lib/util.js | 11 ++++++ .../test-util-decorate-error-stack.js | 35 +++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 test/parallel/test-util-decorate-error-stack.js diff --git a/lib/util.js b/lib/util.js index 25d470fb743dae..63d1fcc765ac7b 100644 --- a/lib/util.js +++ b/lib/util.js @@ -901,3 +901,14 @@ exports._exceptionWithHostPort = function(err, } return ex; }; + + +exports.decorateErrorStack = function(err) { + if (!(isError(err) && err.stack)) + return; + + const arrow = internalUtil.getHiddenValue(err, 'arrowMessage'); + + if (arrow) + err.stack = arrow + err.stack; +}; diff --git a/test/parallel/test-util-decorate-error-stack.js b/test/parallel/test-util-decorate-error-stack.js new file mode 100644 index 00000000000000..b609ee3372574b --- /dev/null +++ b/test/parallel/test-util-decorate-error-stack.js @@ -0,0 +1,35 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const util = require('util'); + +assert.doesNotThrow(function() { + util.decorateErrorStack(); + util.decorateErrorStack(null); + util.decorateErrorStack(1); + util.decorateErrorStack(true); +}); + +// Verify that a stack property is not added to non-Errors +const obj = {}; +util.decorateErrorStack(obj); +assert.strictEqual(obj.stack, undefined); + +// Verify that the stack is decorated when possible +let err; + +try { + require('../fixtures/syntax/bad_syntax'); +} catch (e) { + err = e; + assert(!/var foo bar;/.test(err.stack)); + util.decorateErrorStack(err); +} + +assert(/var foo bar;/.test(err.stack)); + +// Verify that the stack is unchanged when there is no arrow message +err = new Error('foo'); +const originalStack = err.stack; +util.decorateErrorStack(err); +assert.strictEqual(originalStack, err.stack); From 4e23b2a4bfdb1fde47d22931cbc2bcf5967cd850 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 24 Nov 2015 19:55:51 -0500 Subject: [PATCH 3/9] repl: attach location info to syntax errors Currently, when a file with a syntax error is imported in the REPL, no information is provided on the error's location. This commit adds the error's location to the stack trace. Refs: https://github.com/nodejs/node/issues/2762 Refs: https://github.com/nodejs/node/issues/3411 Refs: https://github.com/nodejs/node/issues/3784 PR-URL: https://github.com/nodejs/node/pull/4013 Reviewed-By: Ben Noordhuis --- lib/repl.js | 1 + test/parallel/test-repl-syntax-error-stack.js | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 test/parallel/test-repl-syntax-error-stack.js diff --git a/lib/repl.js b/lib/repl.js index 4fe50f3979e68f..0d82a842be636a 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -274,6 +274,7 @@ function REPLServer(prompt, self._domain.on('error', function(e) { debug('domain error'); const top = replMap.get(self); + util.decorateErrorStack(e); top.outputStream.write((e.stack || e) + '\n'); top.lineParser.reset(); top.bufferedCommand = ''; diff --git a/test/parallel/test-repl-syntax-error-stack.js b/test/parallel/test-repl-syntax-error-stack.js new file mode 100644 index 00000000000000..573059e8d9f34f --- /dev/null +++ b/test/parallel/test-repl-syntax-error-stack.js @@ -0,0 +1,39 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const repl = require('repl'); +const util = require('util'); +let found = false; + +process.on('exit', () => { + assert.strictEqual(found, true); +}); + +// A stream to push an array into a REPL +function ArrayStream() { + this.run = function(data) { + data.forEach(line => { + this.emit('data', line + '\n'); + }); + }; +} +util.inherits(ArrayStream, require('stream').Stream); +ArrayStream.prototype.readable = true; +ArrayStream.prototype.writable = true; +ArrayStream.prototype.resume = function() {}; +ArrayStream.prototype.write = function(output) { + if (/var foo bar;/.test(output)) + found = true; +}; + +const putIn = new ArrayStream(); +const testMe = repl.start('', putIn); +let file = path.resolve(__dirname, '../fixtures/syntax/bad_syntax'); + +if (common.isWindows) + file = file.replace(/\\/g, '\\\\'); + +putIn.run(['.clear']); +putIn.run([`require('${file}');`]); From a8ac55c9c49e19f4034e286a31c61c8d5529a430 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 25 Nov 2015 22:27:29 +0100 Subject: [PATCH 4/9] module,repl: remove repl require() hack Remove a hack that was introduced in commit bb6d468d from November 2010. This is groundwork for a follow-up commit that makes it possible to use internal modules in lib/repl.js. PR-URL: https://github.com/nodejs/node/pull/4026 Reviewed-By: Colin Ihrig Conflicts: lib/module.js --- lib/_debugger.js | 2 +- lib/internal/module.js | 26 ++++++++++++++- lib/module.js | 65 ++++++++++++-------------------------- lib/repl.js | 6 +++- src/node.js | 2 +- test/parallel/test-repl.js | 4 +++ 6 files changed, 56 insertions(+), 49 deletions(-) diff --git a/lib/_debugger.js b/lib/_debugger.js index dd47cbff285149..ad7ee6295bee2a 100644 --- a/lib/_debugger.js +++ b/lib/_debugger.js @@ -5,7 +5,7 @@ const path = require('path'); const net = require('net'); const vm = require('vm'); const Module = require('module'); -const repl = Module.requireRepl(); +const repl = require('repl'); const inherits = util.inherits; const assert = require('assert'); const spawn = require('child_process').spawn; diff --git a/lib/internal/module.js b/lib/internal/module.js index 7f3a39e539424a..ef55aa64bd5642 100644 --- a/lib/internal/module.js +++ b/lib/internal/module.js @@ -1,6 +1,30 @@ 'use strict'; -module.exports.stripBOM = stripBOM; +module.exports = { makeRequireFunction, stripBOM }; + +// Invoke with makeRequireFunction.call(module) where |module| is the +// Module object to use as the context for the require() function. +function makeRequireFunction() { + const Module = this.constructor; + const self = this; + + function require(path) { + return self.require(path); + } + + require.resolve = function(request) { + return Module._resolveFilename(request, self); + }; + + require.main = process.mainModule; + + // Enable support to add extra extension types. + require.extensions = Module._extensions; + + require.cache = Module._cache; + + return require; +} /** * Remove byte order marker. This catches EF BB BF (the UTF-8 BOM) diff --git a/lib/module.js b/lib/module.js index b3d29ffbd1c653..99c476210991fd 100644 --- a/lib/module.js +++ b/lib/module.js @@ -274,17 +274,6 @@ Module._load = function(request, parent, isMain) { debug('Module._load REQUEST %s parent: %s', request, parent.id); } - // REPL is a special case, because it needs the real require. - if (request === 'internal/repl' || request === 'repl') { - if (Module._cache[request]) { - return Module._cache[request]; - } - var replModule = new Module(request); - replModule._compile(NativeModule.getSource(request), `${request}.js`); - NativeModule._cache[request] = replModule; - return replModule.exports; - } - var filename = Module._resolveFilename(request, parent); var cachedModule = Module._cache[filename]; @@ -376,37 +365,9 @@ var resolvedArgv; // the file. // Returns exception, if any. Module.prototype._compile = function(content, filename) { - var self = this; // remove shebang content = content.replace(shebangRe, ''); - function require(path) { - return self.require(path); - } - - require.resolve = function(request) { - return Module._resolveFilename(request, self); - }; - - Object.defineProperty(require, 'paths', { get: function() { - throw new Error('require.paths is removed. Use ' + - 'node_modules folders, or the NODE_PATH ' + - 'environment variable instead.'); - }}); - - require.main = process.mainModule; - - // Enable support to add extra extension types - require.extensions = Module._extensions; - require.registerExtension = function() { - throw new Error('require.registerExtension() removed. Use ' + - 'require.extensions instead.'); - }; - - require.cache = Module._cache; - - var dirname = path.dirname(filename); - // create wrapper function var wrapper = Module.wrap(content); @@ -431,8 +392,22 @@ Module.prototype._compile = function(content, filename) { global.v8debug.Debug.setBreakPoint(compiledWrapper, 0, 0); } } - var args = [self.exports, require, self, filename, dirname]; - return compiledWrapper.apply(self.exports, args); + const dirname = path.dirname(filename); + const require = internalModule.makeRequireFunction.call(this); + + Object.defineProperty(require, 'paths', { get: function() { + throw new Error('require.paths is removed. Use ' + + 'node_modules folders, or the NODE_PATH ' + + 'environment variable instead.'); + }}); + + require.registerExtension = function() { + throw new Error('require.registerExtension() removed. Use ' + + 'require.extensions instead.'); + }; + + const args = [this.exports, require, this, filename, dirname]; + return compiledWrapper.apply(this.exports, args); }; @@ -498,10 +473,10 @@ Module._initPaths = function() { Module.globalPaths = modulePaths.slice(0); }; -// bootstrap repl -Module.requireRepl = function() { - return Module._load('internal/repl', '.'); -}; +// TODO(bnoordhuis) Unused, remove in the future. +Module.requireRepl = internalUtil.deprecate(function() { + return NativeModule.require('internal/repl'); +}, 'Module.requireRepl is deprecated.'); Module._preloadModules = function(requests) { if (!Array.isArray(requests)) diff --git a/lib/repl.js b/lib/repl.js index 0d82a842be636a..42fb6ad3405a74 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -21,6 +21,7 @@ 'use strict'; +const internalModule = require('internal/module'); const util = require('util'); const inherits = util.inherits; const Stream = require('stream'); @@ -29,6 +30,7 @@ const path = require('path'); const fs = require('fs'); const rl = require('readline'); const Console = require('console').Console; +const Module = require('module'); const domain = require('domain'); const debug = util.debuglog('repl'); @@ -508,6 +510,8 @@ REPLServer.prototype.createContext = function() { context.global.global = context; } + const module = new Module(''); + const require = internalModule.makeRequireFunction.call(module); context.module = module; context.require = require; @@ -647,7 +651,7 @@ REPLServer.prototype.complete = function(line, callback) { completionGroupsLoaded(); } else if (match = line.match(requireRE)) { // require('...') - var exts = Object.keys(require.extensions); + const exts = Object.keys(this.context.require.extensions); var indexRe = new RegExp('^index(' + exts.map(regexpEscape).join('|') + ')$'); diff --git a/src/node.js b/src/node.js index a9a85809096a6e..367f68b25c003c 100644 --- a/src/node.js +++ b/src/node.js @@ -142,7 +142,7 @@ // If -i or --interactive were passed, or stdin is a TTY. if (process._forceRepl || NativeModule.require('tty').isatty(0)) { // REPL - var cliRepl = Module.requireRepl(); + var cliRepl = NativeModule.require('internal/repl'); cliRepl.createInternalRepl(process.env, function(err, repl) { if (err) { throw err; diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index 4d65a1e9d084b4..0d05de9bc1863d 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -278,6 +278,10 @@ function error_test() { expect: 'undefined\n' + prompt_unix }, { client: client_unix, send: '/* \'\n"\n\'"\'\n*/', expect: 'undefined\n' + prompt_unix }, + // REPL should get a normal require() function, not one that allows + // access to internal modules without the --expose_internals flag. + { client: client_unix, send: 'require("internal/repl")', + expect: /^Error: Cannot find module 'internal\/repl'/ }, ]); } From c16fa6b37536cee8503197d1342978505d6da6f9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 25 Nov 2015 22:37:43 +0100 Subject: [PATCH 5/9] util: move .decorateErrorStack to internal/util Move the method that was added in commit 8ca412b from earlier this month from lib/util.js to lib/internal/util.js. Avoids exposing a method that we may not wish to expose just yet, seeing how it relies on implementation details. PR-URL: https://github.com/nodejs/node/pull/4026 Reviewed-By: Colin Ihrig --- lib/internal/util.js | 18 ++++++++++++++++ lib/repl.js | 3 ++- lib/util.js | 21 +++---------------- .../test-util-decorate-error-stack.js | 17 ++++++++------- 4 files changed, 32 insertions(+), 27 deletions(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index 5be073f18a2217..c34682ed1b3809 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -57,3 +57,21 @@ exports._deprecate = function(fn, msg) { return deprecated; }; + +exports.decorateErrorStack = function decorateErrorStack(err) { + if (!(exports.isError(err) && err.stack)) + return; + + const arrow = exports.getHiddenValue(err, 'arrowMessage'); + + if (arrow) + err.stack = arrow + err.stack; +}; + +exports.isError = function isError(e) { + return exports.objectToString(e) === '[object Error]' || e instanceof Error; +}; + +exports.objectToString = function objectToString(o) { + return Object.prototype.toString.call(o); +}; diff --git a/lib/repl.js b/lib/repl.js index 42fb6ad3405a74..80ec4defaf1646 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -22,6 +22,7 @@ 'use strict'; const internalModule = require('internal/module'); +const internalUtil = require('internal/util'); const util = require('util'); const inherits = util.inherits; const Stream = require('stream'); @@ -276,7 +277,7 @@ function REPLServer(prompt, self._domain.on('error', function(e) { debug('domain error'); const top = replMap.get(self); - util.decorateErrorStack(e); + internalUtil.decorateErrorStack(e); top.outputStream.write((e.stack || e) + '\n'); top.lineParser.reset(); top.bufferedCommand = ''; diff --git a/lib/util.js b/lib/util.js index 63d1fcc765ac7b..1a2f3f34835899 100644 --- a/lib/util.js +++ b/lib/util.js @@ -5,6 +5,9 @@ const Buffer = require('buffer').Buffer; const internalUtil = require('internal/util'); const binding = process.binding('util'); +const isError = internalUtil.isError; +const objectToString = internalUtil.objectToString; + var Debug; const formatRegExp = /%[sdj%]/g; @@ -684,9 +687,6 @@ function isDate(d) { } exports.isDate = isDate; -function isError(e) { - return objectToString(e) === '[object Error]' || e instanceof Error; -} exports.isError = isError; function isFunction(arg) { @@ -702,10 +702,6 @@ exports.isPrimitive = isPrimitive; exports.isBuffer = Buffer.isBuffer; -function objectToString(o) { - return Object.prototype.toString.call(o); -} - function pad(n) { return n < 10 ? '0' + n.toString(10) : n.toString(10); @@ -901,14 +897,3 @@ exports._exceptionWithHostPort = function(err, } return ex; }; - - -exports.decorateErrorStack = function(err) { - if (!(isError(err) && err.stack)) - return; - - const arrow = internalUtil.getHiddenValue(err, 'arrowMessage'); - - if (arrow) - err.stack = arrow + err.stack; -}; diff --git a/test/parallel/test-util-decorate-error-stack.js b/test/parallel/test-util-decorate-error-stack.js index b609ee3372574b..24fee56df7b655 100644 --- a/test/parallel/test-util-decorate-error-stack.js +++ b/test/parallel/test-util-decorate-error-stack.js @@ -1,18 +1,19 @@ +// Flags: --expose_internals 'use strict'; const common = require('../common'); const assert = require('assert'); -const util = require('util'); +const internalUtil = require('internal/util'); assert.doesNotThrow(function() { - util.decorateErrorStack(); - util.decorateErrorStack(null); - util.decorateErrorStack(1); - util.decorateErrorStack(true); + internalUtil.decorateErrorStack(); + internalUtil.decorateErrorStack(null); + internalUtil.decorateErrorStack(1); + internalUtil.decorateErrorStack(true); }); // Verify that a stack property is not added to non-Errors const obj = {}; -util.decorateErrorStack(obj); +internalUtil.decorateErrorStack(obj); assert.strictEqual(obj.stack, undefined); // Verify that the stack is decorated when possible @@ -23,7 +24,7 @@ try { } catch (e) { err = e; assert(!/var foo bar;/.test(err.stack)); - util.decorateErrorStack(err); + internalUtil.decorateErrorStack(err); } assert(/var foo bar;/.test(err.stack)); @@ -31,5 +32,5 @@ assert(/var foo bar;/.test(err.stack)); // Verify that the stack is unchanged when there is no arrow message err = new Error('foo'); const originalStack = err.stack; -util.decorateErrorStack(err); +internalUtil.decorateErrorStack(err); assert.strictEqual(originalStack, err.stack); From f64aa34ebd3285b665917a17f040fd272de53be7 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 25 Nov 2015 16:38:01 -0500 Subject: [PATCH 6/9] test: move ArrayStream to common A number of REPL tests define the same ArrayStream object. This commit moves the repeated code into common.js. PR-URL: https://github.com/nodejs/node/pull/4027 Reviewed-By: Ben Noordhuis Conflicts: test/common.js --- test/common.js | 19 ++++++++++++++++++ test/parallel/test-repl-.save.load.js | 17 +--------------- test/parallel/test-repl-autolibs.js | 20 ++++--------------- test/parallel/test-repl-console.js | 7 ++----- test/parallel/test-repl-domain.js | 17 +--------------- test/parallel/test-repl-end-emits-exit.js | 7 ++----- test/parallel/test-repl-options.js | 7 ++----- test/parallel/test-repl-reset-event.js | 7 ++----- test/parallel/test-repl-syntax-error-stack.js | 16 ++------------- test/parallel/test-repl-tab-complete-crash.js | 19 +++--------------- test/parallel/test-repl-tab-complete.js | 17 +--------------- 11 files changed, 39 insertions(+), 114 deletions(-) diff --git a/test/common.js b/test/common.js index 0e824efd94fe6d..c1012e7a097688 100644 --- a/test/common.js +++ b/test/common.js @@ -5,6 +5,8 @@ var fs = require('fs'); var assert = require('assert'); var os = require('os'); var child_process = require('child_process'); +const stream = require('stream'); +const util = require('util'); exports.testDir = path.dirname(__filename); @@ -503,3 +505,20 @@ exports.nodeProcessAborted = function nodeProcessAborted(exitCode, signal) { return expectedExitCodes.indexOf(exitCode) > -1; } }; + +// A stream to push an array into a REPL +function ArrayStream() { + this.run = function(data) { + data.forEach(line => { + this.emit('data', line + '\n'); + }); + }; +} + +util.inherits(ArrayStream, stream.Stream); +exports.ArrayStream = ArrayStream; +ArrayStream.prototype.readable = true; +ArrayStream.prototype.writable = true; +ArrayStream.prototype.pause = function() {}; +ArrayStream.prototype.resume = function() {}; +ArrayStream.prototype.write = function() {}; diff --git a/test/parallel/test-repl-.save.load.js b/test/parallel/test-repl-.save.load.js index 56566719926519..c71b383bcd7409 100644 --- a/test/parallel/test-repl-.save.load.js +++ b/test/parallel/test-repl-.save.load.js @@ -9,24 +9,9 @@ common.refreshTmpDir(); var repl = require('repl'); -// A stream to push an array into a REPL -function ArrayStream() { - this.run = function(data) { - var self = this; - data.forEach(function(line) { - self.emit('data', line + '\n'); - }); - }; -} -util.inherits(ArrayStream, require('stream').Stream); -ArrayStream.prototype.readable = true; -ArrayStream.prototype.writable = true; -ArrayStream.prototype.resume = function() {}; -ArrayStream.prototype.write = function() {}; - var works = [['inner.one'], 'inner.o']; -var putIn = new ArrayStream(); +const putIn = new common.ArrayStream(); var testMe = repl.start('', putIn); diff --git a/test/parallel/test-repl-autolibs.js b/test/parallel/test-repl-autolibs.js index e37f2d036ece4d..05cc299f56888f 100644 --- a/test/parallel/test-repl-autolibs.js +++ b/test/parallel/test-repl-autolibs.js @@ -1,25 +1,13 @@ -/* eslint-disable required-modules */ 'use strict'; +const common = require('../common'); var assert = require('assert'); var util = require('util'); var repl = require('repl'); -// A stream to push an array into a REPL -function ArrayStream() { - this.run = function(data) { - var self = this; - data.forEach(function(line) { - self.emit('data', line + '\n'); - }); - }; -} -util.inherits(ArrayStream, require('stream').Stream); -ArrayStream.prototype.readable = true; -ArrayStream.prototype.writable = true; -ArrayStream.prototype.resume = function() {}; -ArrayStream.prototype.write = function() {}; +// This test adds global variables +common.globalCheck = false; -var putIn = new ArrayStream(); +const putIn = new common.ArrayStream(); var testMe = repl.start('', putIn, null, true); test1(); diff --git a/test/parallel/test-repl-console.js b/test/parallel/test-repl-console.js index e66fcb1621adc5..fe737d841e1651 100644 --- a/test/parallel/test-repl-console.js +++ b/test/parallel/test-repl-console.js @@ -1,13 +1,10 @@ 'use strict'; var common = require('../common'), assert = require('assert'), - Stream = require('stream'), repl = require('repl'); -// create a dummy stream that does nothing -var stream = new Stream(); -stream.write = stream.pause = stream.resume = function() {}; -stream.readable = stream.writable = true; +// Create a dummy stream that does nothing +const stream = new common.ArrayStream(); var r = repl.start({ input: stream, diff --git a/test/parallel/test-repl-domain.js b/test/parallel/test-repl-domain.js index 7528f502878f63..baf0485b3b80b2 100644 --- a/test/parallel/test-repl-domain.js +++ b/test/parallel/test-repl-domain.js @@ -5,22 +5,7 @@ var common = require('../common'); var util = require('util'); var repl = require('repl'); -// A stream to push an array into a REPL -function ArrayStream() { - this.run = function(data) { - var self = this; - data.forEach(function(line) { - self.emit('data', line + '\n'); - }); - }; -} -util.inherits(ArrayStream, require('stream').Stream); -ArrayStream.prototype.readable = true; -ArrayStream.prototype.writable = true; -ArrayStream.prototype.resume = function() {}; -ArrayStream.prototype.write = function() {}; - -var putIn = new ArrayStream(); +const putIn = new common.ArrayStream(); var testMe = repl.start('', putIn); putIn.write = function(data) { diff --git a/test/parallel/test-repl-end-emits-exit.js b/test/parallel/test-repl-end-emits-exit.js index e4bc4da78c142d..2c18b413d68c0f 100644 --- a/test/parallel/test-repl-end-emits-exit.js +++ b/test/parallel/test-repl-end-emits-exit.js @@ -1,15 +1,12 @@ 'use strict'; var common = require('../common'), assert = require('assert'), - Stream = require('stream'), repl = require('repl'), terminalExit = 0, regularExit = 0; -// create a dummy stream that does nothing -var stream = new Stream(); -stream.write = stream.pause = stream.resume = function() {}; -stream.readable = stream.writable = true; +// Create a dummy stream that does nothing +const stream = new common.ArrayStream(); function testTerminalMode() { var r1 = repl.start({ diff --git a/test/parallel/test-repl-options.js b/test/parallel/test-repl-options.js index 5bc37d2d7117f3..ec3b144ec0ef77 100644 --- a/test/parallel/test-repl-options.js +++ b/test/parallel/test-repl-options.js @@ -1,15 +1,12 @@ 'use strict'; var common = require('../common'), assert = require('assert'), - Stream = require('stream'), repl = require('repl'); common.globalCheck = false; -// create a dummy stream that does nothing -var stream = new Stream(); -stream.write = stream.pause = stream.resume = function() {}; -stream.readable = stream.writable = true; +// Create a dummy stream that does nothing +const stream = new common.ArrayStream(); // 1, mostly defaults var r1 = repl.start({ diff --git a/test/parallel/test-repl-reset-event.js b/test/parallel/test-repl-reset-event.js index e6d4eed1385b10..0bd43dcd6c370e 100644 --- a/test/parallel/test-repl-reset-event.js +++ b/test/parallel/test-repl-reset-event.js @@ -4,12 +4,9 @@ common.globalCheck = false; var assert = require('assert'); var repl = require('repl'); -var Stream = require('stream'); -// create a dummy stream that does nothing -var dummy = new Stream(); -dummy.write = dummy.pause = dummy.resume = function() {}; -dummy.readable = dummy.writable = true; +// Create a dummy stream that does nothing +const dummy = new common.ArrayStream(); function testReset(cb) { var r = repl.start({ diff --git a/test/parallel/test-repl-syntax-error-stack.js b/test/parallel/test-repl-syntax-error-stack.js index 573059e8d9f34f..647d3e3569ce00 100644 --- a/test/parallel/test-repl-syntax-error-stack.js +++ b/test/parallel/test-repl-syntax-error-stack.js @@ -11,24 +11,12 @@ process.on('exit', () => { assert.strictEqual(found, true); }); -// A stream to push an array into a REPL -function ArrayStream() { - this.run = function(data) { - data.forEach(line => { - this.emit('data', line + '\n'); - }); - }; -} -util.inherits(ArrayStream, require('stream').Stream); -ArrayStream.prototype.readable = true; -ArrayStream.prototype.writable = true; -ArrayStream.prototype.resume = function() {}; -ArrayStream.prototype.write = function(output) { +common.ArrayStream.prototype.write = function(output) { if (/var foo bar;/.test(output)) found = true; }; -const putIn = new ArrayStream(); +const putIn = new common.ArrayStream(); const testMe = repl.start('', putIn); let file = path.resolve(__dirname, '../fixtures/syntax/bad_syntax'); diff --git a/test/parallel/test-repl-tab-complete-crash.js b/test/parallel/test-repl-tab-complete-crash.js index 85ab0577eb8601..484580f1e72fe5 100644 --- a/test/parallel/test-repl-tab-complete-crash.js +++ b/test/parallel/test-repl-tab-complete-crash.js @@ -1,32 +1,19 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const util = require('util'); const repl = require('repl'); var referenceErrorCount = 0; -// A stream to push an array into a REPL -function ArrayStream() { - this.run = function(data) { - const self = this; - data.forEach(function(line) { - self.emit('data', line + '\n'); - }); - }; -} -util.inherits(ArrayStream, require('stream').Stream); -ArrayStream.prototype.readable = true; -ArrayStream.prototype.writable = true; -ArrayStream.prototype.resume = function() {}; -ArrayStream.prototype.write = function(msg) { +common.ArrayStream.prototype.write = function(msg) { if (msg.startsWith('ReferenceError: ')) { referenceErrorCount++; } }; -const putIn = new ArrayStream(); +const putIn = new common.ArrayStream(); const testMe = repl.start('', putIn); // https://github.com/nodejs/node/issues/3346 diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index 1cb6e7791d3c2b..6bf7d5059d0116 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -20,23 +20,8 @@ process.on('exit', function() { assert.strictEqual(referenceErrors, expectedReferenceErrors); }); -// A stream to push an array into a REPL -function ArrayStream() { - this.run = function(data) { - var self = this; - data.forEach(function(line) { - self.emit('data', line + '\n'); - }); - }; -} -util.inherits(ArrayStream, require('stream').Stream); -ArrayStream.prototype.readable = true; -ArrayStream.prototype.writable = true; -ArrayStream.prototype.resume = function() {}; -ArrayStream.prototype.write = function() {}; - var works = [['inner.one'], 'inner.o']; -var putIn = new ArrayStream(); +const putIn = new common.ArrayStream(); var testMe = repl.start('', putIn); // Some errors are passed to the domain, but do not callback From b5f8a359c02abaa606039e79838c036f736eb1f3 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 1 Dec 2015 11:27:39 -0500 Subject: [PATCH 7/9] util: determine object types in C++ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Determine object types of regular expressions, Dates, Maps, and Sets in the C++ layer instead of depending on toString() behavior in JavaScript. PR-URL: https://github.com/nodejs/node/pull/4100 Reviewed-By: Evan Lucas Reviewed-By: Michaël Zasso Reviewed-By: Ben Noordhuis --- lib/util.js | 9 ++++----- src/node_util.cc | 29 +++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/util.js b/lib/util.js index 1a2f3f34835899..236adf35b87eab 100644 --- a/lib/util.js +++ b/lib/util.js @@ -6,7 +6,6 @@ const internalUtil = require('internal/util'); const binding = process.binding('util'); const isError = internalUtil.isError; -const objectToString = internalUtil.objectToString; var Debug; @@ -305,7 +304,7 @@ function formatValue(ctx, value, recurseTimes) { braces = ['[', ']']; empty = value.length === 0; formatter = formatArray; - } else if (objectToString(value) === '[object Set]') { + } else if (binding.isSet(value)) { braces = ['{', '}']; // With `showHidden`, `length` will display as a hidden property for // arrays. For consistency's sake, do the same for `size`, even though this @@ -314,7 +313,7 @@ function formatValue(ctx, value, recurseTimes) { keys.unshift('size'); empty = value.size === 0; formatter = formatSet; - } else if (objectToString(value) === '[object Map]') { + } else if (binding.isMap(value)) { braces = ['{', '}']; // Ditto. if (ctx.showHidden) @@ -673,7 +672,7 @@ function isUndefined(arg) { exports.isUndefined = isUndefined; function isRegExp(re) { - return objectToString(re) === '[object RegExp]'; + return binding.isRegExp(re); } exports.isRegExp = isRegExp; @@ -683,7 +682,7 @@ function isObject(arg) { exports.isObject = isObject; function isDate(d) { - return objectToString(d) === '[object Date]'; + return binding.isDate(d); } exports.isDate = isDate; diff --git a/src/node_util.cc b/src/node_util.cc index a520b8d5f354ac..ea800ff339a36f 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -13,12 +13,37 @@ using v8::Object; using v8::String; using v8::Value; + +static void IsRegExp(const FunctionCallbackInfo& args) { + CHECK_EQ(1, args.Length()); + args.GetReturnValue().Set(args[0]->IsRegExp()); +} + + +static void IsDate(const FunctionCallbackInfo& args) { + CHECK_EQ(1, args.Length()); + args.GetReturnValue().Set(args[0]->IsDate()); +} + + +static void IsMap(const FunctionCallbackInfo& args) { + CHECK_EQ(1, args.Length()); + args.GetReturnValue().Set(args[0]->IsMap()); +} + + static void IsMapIterator(const FunctionCallbackInfo& args) { CHECK_EQ(1, args.Length()); args.GetReturnValue().Set(args[0]->IsMapIterator()); } +static void IsSet(const FunctionCallbackInfo& args) { + CHECK_EQ(1, args.Length()); + args.GetReturnValue().Set(args[0]->IsSet()); +} + + static void IsSetIterator(const FunctionCallbackInfo& args) { CHECK_EQ(1, args.Length()); args.GetReturnValue().Set(args[0]->IsSetIterator()); @@ -50,7 +75,11 @@ void Initialize(Local target, Local unused, Local context) { Environment* env = Environment::GetCurrent(context); + env->SetMethod(target, "isRegExp", IsRegExp); + env->SetMethod(target, "isDate", IsDate); + env->SetMethod(target, "isMap", IsMap); env->SetMethod(target, "isMapIterator", IsMapIterator); + env->SetMethod(target, "isSet", IsSet); env->SetMethod(target, "isSetIterator", IsSetIterator); env->SetMethod(target, "isPromise", IsPromise); env->SetMethod(target, "getHiddenValue", GetHiddenValue); From 1782577d9401b45717a385607175c66d2578c666 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 2 Dec 2015 11:49:35 -0500 Subject: [PATCH 8/9] src: define Is* util functions with macros The Is* type checking functions in node_util.cc are mostly the same boilerplate. This commit defines them using a macro. Refs: https://github.com/nodejs/node/pull/4100 PR-URL: https://github.com/nodejs/node/pull/4118 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Conflicts: src/node_util.cc --- src/node_util.cc | 66 +++++++++++++++++------------------------------- 1 file changed, 23 insertions(+), 43 deletions(-) diff --git a/src/node_util.cc b/src/node_util.cc index ea800ff339a36f..1e0f214ae4470a 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -14,45 +14,27 @@ using v8::String; using v8::Value; -static void IsRegExp(const FunctionCallbackInfo& args) { - CHECK_EQ(1, args.Length()); - args.GetReturnValue().Set(args[0]->IsRegExp()); -} - +#define VALUE_METHOD_MAP(V) \ + V(isArrayBuffer, IsArrayBuffer) \ + V(isDataView, IsDataView) \ + V(isDate, IsDate) \ + V(isMap, IsMap) \ + V(isMapIterator, IsMapIterator) \ + V(isPromise, IsPromise) \ + V(isRegExp, IsRegExp) \ + V(isSet, IsSet) \ + V(isSetIterator, IsSetIterator) \ + V(isTypedArray, IsTypedArray) -static void IsDate(const FunctionCallbackInfo& args) { - CHECK_EQ(1, args.Length()); - args.GetReturnValue().Set(args[0]->IsDate()); -} +#define V(_, ucname) \ + static void ucname(const FunctionCallbackInfo& args) { \ + CHECK_EQ(1, args.Length()); \ + args.GetReturnValue().Set(args[0]->ucname()); \ + } -static void IsMap(const FunctionCallbackInfo& args) { - CHECK_EQ(1, args.Length()); - args.GetReturnValue().Set(args[0]->IsMap()); -} - - -static void IsMapIterator(const FunctionCallbackInfo& args) { - CHECK_EQ(1, args.Length()); - args.GetReturnValue().Set(args[0]->IsMapIterator()); -} - - -static void IsSet(const FunctionCallbackInfo& args) { - CHECK_EQ(1, args.Length()); - args.GetReturnValue().Set(args[0]->IsSet()); -} - - -static void IsSetIterator(const FunctionCallbackInfo& args) { - CHECK_EQ(1, args.Length()); - args.GetReturnValue().Set(args[0]->IsSetIterator()); -} - -static void IsPromise(const FunctionCallbackInfo& args) { - CHECK_EQ(1, args.Length()); - args.GetReturnValue().Set(args[0]->IsPromise()); -} + VALUE_METHOD_MAP(V) +#undef V static void GetHiddenValue(const FunctionCallbackInfo& args) { @@ -75,13 +57,11 @@ void Initialize(Local target, Local unused, Local context) { Environment* env = Environment::GetCurrent(context); - env->SetMethod(target, "isRegExp", IsRegExp); - env->SetMethod(target, "isDate", IsDate); - env->SetMethod(target, "isMap", IsMap); - env->SetMethod(target, "isMapIterator", IsMapIterator); - env->SetMethod(target, "isSet", IsSet); - env->SetMethod(target, "isSetIterator", IsSetIterator); - env->SetMethod(target, "isPromise", IsPromise); + +#define V(lcname, ucname) env->SetMethod(target, #lcname, ucname); + VALUE_METHOD_MAP(V) +#undef V + env->SetMethod(target, "getHiddenValue", GetHiddenValue); } From 82c9778ab0817d2a4bb76ddfa61fc36a1feddc91 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 9 Dec 2015 21:55:29 +0100 Subject: [PATCH 9/9] repl: fix require('3rdparty') regression Fix module loading of third-party modules in the REPL by inheriting module.paths from the REPL's parent module. Commit ee72ee7 ("module,repl: remove repl require() hack") introduced a regression where require() of modules in node_modules directories no longer worked in the REPL (and fortunately only in the REPL.) It turns out we didn't have test coverage for that but we do now. Fixes: https://github.com/nodejs/node/issues/4208 PR-URL: https://github.com/nodejs/node/pull/4215 Reviewed-By: Roman Reiss --- lib/repl.js | 3 +++ test/fixtures/node_modules/baz/index.js | 2 ++ test/parallel/test-repl-require.js | 33 +++++++++++++++++++++++++ 3 files changed, 38 insertions(+) create mode 100644 test/parallel/test-repl-require.js diff --git a/lib/repl.js b/lib/repl.js index 80ec4defaf1646..84ac726e1f1671 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -35,6 +35,7 @@ const Module = require('module'); const domain = require('domain'); const debug = util.debuglog('repl'); +const parentModule = module; const replMap = new WeakMap(); try { @@ -512,6 +513,8 @@ REPLServer.prototype.createContext = function() { } const module = new Module(''); + module.paths = Module._resolveLookupPaths('', parentModule)[1]; + const require = internalModule.makeRequireFunction.call(module); context.module = module; context.require = require; diff --git a/test/fixtures/node_modules/baz/index.js b/test/fixtures/node_modules/baz/index.js index 346f068ec52a2b..859ea0924d6fb7 100644 --- a/test/fixtures/node_modules/baz/index.js +++ b/test/fixtures/node_modules/baz/index.js @@ -6,3 +6,5 @@ assert.equal(require('bar'), require('../bar.js')); // this should work, and get the one in ./node_modules/asdf.js assert.equal(require('asdf'), require('./node_modules/asdf.js')); + +module.exports = 'eye catcher'; diff --git a/test/parallel/test-repl-require.js b/test/parallel/test-repl-require.js new file mode 100644 index 00000000000000..e8e5e34190c464 --- /dev/null +++ b/test/parallel/test-repl-require.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); + +process.chdir(common.fixturesDir); +const repl = require('repl'); + +const server = net.createServer(conn => { + repl.start('', conn).on('exit', () => { + conn.destroy(); + server.close(); + }); +}); + +const host = common.localhostIPv4; +const port = common.PORT; +const options = { host, port }; + +var answer = ''; +server.listen(options, function() { + const conn = net.connect(options); + conn.setEncoding('utf8'); + conn.on('data', data => answer += data); + conn.write('require("baz")\n.exit\n'); +}); + +process.on('exit', function() { + assert.strictEqual(false, /Cannot find module/.test(answer)); + assert.strictEqual(false, /Error/.test(answer)); + assert.strictEqual(true, /eye catcher/.test(answer)); +});