Skip to content

errors, child_process: migrate to using internal/errors #11300

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
jasnell wants to merge 2 commits into from
Closed

errors, child_process: migrate to using internal/errors #11300

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
896e8db
errors: use lazy assert to avoid issues on startup
jasnell Apr 20, 2017
14cacc6
errors, child_process: migrate to using internal/errors
jasnell Feb 10, 2017
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
Next Next commit
errors: use lazy assert to avoid issues on startup 
Use of assert must be lazy to allow errors to be used early
before the process is completely set up
  • Loading branch information
@jasnell
jasnell committed Apr 27, 2017
commit 896e8dbbebbdee440762fba9323bd320165ef7bf
11 changes: 9 additions & 2 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
// value statically and permanently identifies the error. While the error
// message may change, the code should not.

const assert = require('assert');
const kCode = Symbol('code');
const messages = new Map();

Expand All @@ -17,6 +16,13 @@ function lazyUtil() {
return util;
}

var assert;
function lazyAssert() {
if (!assert)
assert = require('assert');
return assert;
}

function makeNodeError(Base) {
return class NodeError extends Base {
constructor(key, ...args) {
Expand All @@ -36,6 +42,7 @@ function makeNodeError(Base) {
}

function message(key, args) {
const assert = lazyAssert();
assert.strictEqual(typeof key, 'string');
const util = lazyUtil();
const msg = messages.get(key);
Expand All @@ -54,7 +61,6 @@ function message(key, args) {
// Utility function for registering the error codes. Only used here. Exported
// *only* to allow for testing.
function E(sym, val) {
assert(messages.has(sym) === false, `Error symbol: ${sym} was already used.`);
Copy link
Copy Markdown
Member

@targos targos Apr 24, 2017

Choose a reason for hiding this comment

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

Why did you remove this check? (The test for it is removed in the second commit btw)

Copy link
Copy Markdown
Member Author

@jasnell jasnell Apr 24, 2017

Choose a reason for hiding this comment

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

Because of issues in the loading order of the errors.js module relative to assert.js on startup. There is a circular dependency that happens that causes the util.js used within assert.js to fail depending on when assert is loaded. I could replace this with a simple throw rather than the call to assert if you'd be more comfortable with that.

Copy link
Copy Markdown
Member

@targos targos Apr 24, 2017

Choose a reason for hiding this comment

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

I'm fine with removing it. I was just curious. The risk of reusing the same code is low if we keep them together and in alphabetical order.

messages.set(sym, typeof val === 'function' ? val : String(val));
}

Expand Down Expand Up @@ -99,6 +105,7 @@ E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
// Add new errors from here...

function invalidArgType(name, expected, actual) {
Copy link
Copy Markdown
Member

@targos targos Mar 23, 2017

Choose a reason for hiding this comment

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

If I read it correctly, this would print The "options" argument must be type Object. It looks better to me as The "options" argument must be of type Object

Copy link
Copy Markdown
Member

@targos targos Mar 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It would be nice to have some tests for this function OK, tests are in #11294

const assert = lazyAssert();
assert(name, 'name is required');
var msg = `The "${name}" argument must be ${oneOf(expected, 'type')}`;
if (arguments.length >= 3) {
Expand Down
6 changes: 0 additions & 6 deletions test/parallel/test-internal-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,6 @@ assert.throws(() => {
message: /^Error for testing 2/ }));
}, /AssertionError: .+ does not match \S/);

assert.doesNotThrow(() => errors.E('TEST_ERROR_USED_SYMBOL'));
Copy link
Copy Markdown
Member

@targos targos Apr 24, 2017

Choose a reason for hiding this comment

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

please bundle this change with the same commit that removed the error

assert.throws(
() => errors.E('TEST_ERROR_USED_SYMBOL'),
/^AssertionError: Error symbol: TEST_ERROR_USED_SYMBOL was already used\.$/
);

// // Test ERR_INVALID_ARG_TYPE
assert.strictEqual(errors.message('ERR_INVALID_ARG_TYPE', ['a', 'b']),
'The "a" argument must be of type b');
Expand Down