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

errors: add internal/errors.js #9265

Closed
wants to merge 9 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 24, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

errors

Description of change

A more incremental version of #6573.
Introduces the internal/errors.js module and begins updating errors in the
internal modules to use it. This is intentionally incomplete. Other errors can
be migrated over to this incrementally.

This is semver-major because several of the error messages are updated
in the process.

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. error-handling errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Oct 24, 2016
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 24, 2016
const kCode = Symbol('code');

class NodeError extends Error {
constructor(key, ...args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we replace all of the rest args usage with a copy loop or whatever is fastest currently?

if (flags & binding.URL_FLAGS_FAILED)
throw new TypeError('Invalid URL');
if (flags & binding.URL_FLAGS_FAILED) {
const errors = require('internal/errors');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be placed at the top of the file?

if (noCrypto)
throw new Error('Node.js is not compiled with openssl crypto support');
if (noCrypto) {
const errors = require('internal/errors');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto?

@jasnell
Copy link
Member Author

jasnell commented Oct 25, 2016

@mscdex ... updated!

@jasnell
Copy link
Member Author

jasnell commented Oct 25, 2016

Can. Kept it local to the error since it's the only instance of use.

On Monday, October 24, 2016, Brian White [email protected] wrote:

@mscdex commented on this pull request.

In lib/internal/url.js
#9265 (review):

@@ -81,8 +81,10 @@ class URL {
binding.parse(input.trim(), -1, base_context, undefined,
(flags, protocol, username, password,
host, port, path, query, fragment) => {

  •    if (flags & binding.URL_FLAGS_FAILED)
    
  •      throw new TypeError('Invalid URL');
    
  •    if (flags & binding.URL_FLAGS_FAILED) {
    
  •      const errors = require('internal/errors');
    

Can't this be placed at the top of the file?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9265 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eVKFdAy4Tw_MMVnPg8ju5c2C39Pvks5q3UiDgaJpZM4KfZwM
.


// Utility function for registering the error codes.
function E(sym, val) {
exports[Symbol.for(String(sym).toUpperCase())] =
Copy link
Member

Choose a reason for hiding this comment

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

Since this is using the global symbol registry, perhaps we should add a NODE_ prefix to the key.

// to the readable-stream standalone module. If the logic here changes at all,
// then those also need to be checked.

const kCode = Symbol('code');
Copy link
Member

Choose a reason for hiding this comment

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

Symbol('error code') ?

const kCode = Symbol('code');

class NodeError extends Error {
constructor(key /**, ...args **/) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the overhead of rest parameters is negligible here.
Testcase: https://jsperf.com/custom-error-with-rest-params

@@ -415,7 +415,8 @@
}

if (!NativeModule.exists(id)) {
throw new Error(`No such native module ${id}`);
const errors = require('internal/errors');
Copy link
Member

Choose a reason for hiding this comment

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

nit: const {Error} = require('internal/errors');

@jasnell jasnell mentioned this pull request Feb 7, 2017
4 tasks
@jasnell jasnell closed this Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants