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: move error codes into separate module #14250

Closed
wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Jul 15, 2017

Move error codes into a new separate module internal/error_codes. And the guides/using-internal-errors.md are also changed.

This PR only separate the error codes in internal/errors.js. I think the rest of error codes in ./lib should wait for some future PRs.

Ref: #14216, #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

errors

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jul 15, 2017
@Trott
Copy link
Member

Trott commented Jul 15, 2017

Would it be better to create an internal/errors directory, move internal/errors.js to internal/errors/index.js and put the codes in internal/errors/codes.js?

EXAMPLE_KEY1: 'EXAMPLE_KEY1',
EXAMPLE_KEY2: 'EXAMPLE_KEY2',
// ...
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Linting nit: missing semicolon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@starkwang starkwang force-pushed the separate-error-codes branch 2 times, most recently from 30a8307 to 7610dbe Compare July 15, 2017 14:21
@vsemozhetbyt vsemozhetbyt added the doc Issues and PRs related to the documentations. label Jul 15, 2017
@starkwang
Copy link
Contributor Author

starkwang commented Jul 15, 2017

@Trott I tried to create an internal/errors directory (with index.js and codes.js in it), but found the module system failed to load internal/errors/index.js whenrequire('internal/errors')

Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: internal/errors

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@starkwang Ah! It looks like we may have run into a similar issue with internal/process and we just left lib/internal/process.js and put other files in lib/internal/process/. So maybe you can do that. (In other words, leave internal/errors.js where it is now but create internal/errors directory and put codes.js in there.)

@starkwang starkwang force-pushed the separate-error-codes branch from 7610dbe to ef6c0c2 Compare July 15, 2017 15:01
@starkwang
Copy link
Contributor Author

Pushed commit to address comments.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Needs one small doc change but otherwise looks good to me if the CI is green.

@@ -47,12 +47,22 @@ const err = new errors.TypeError('FOO', type);

## Adding new errors

New static error codes are added by modifying the `internal/errors.js` file
and appending the new error codes to the end using the utility `E()` method.
New static error codes are added by modifying the `internal/error_codes.js` file.
Copy link
Member

Choose a reason for hiding this comment

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

The path here needs to be updated to reflect the new location of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc fixed.

@Trott
Copy link
Member

Trott commented Jul 15, 2017

/cc @jasnell

@starkwang starkwang force-pushed the separate-error-codes branch from ef6c0c2 to dc185e6 Compare July 15, 2017 15:30
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

@starkwang starkwang force-pushed the separate-error-codes branch 3 times, most recently from 9210f2d to 93ec771 Compare July 18, 2017 07:56
@starkwang starkwang force-pushed the separate-error-codes branch from 93ec771 to 7576dd4 Compare July 18, 2017 08:04
@joyeecheung
Copy link
Member

Ping @starkwang, this needs a rebase.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

FWIW, I am -1 on this approach as it currently stands. There is no reason to move the codes to a separate file and doing so now would unnecessarily disrupt the current ongoing migration process. If we want to expose the codes as constants, then those can be dropped off the existing internal/errors module.exports target.

@refack
Copy link
Contributor

refack commented Jul 24, 2017

FWIW, I am -1 on this approach as it currently stands. There is no reason to move the codes to a separate file and doing so now would unnecessarily disrupt the current ongoing migration process. If we want to expose the codes as constants, then those can be dropped off the existing internal/errors module.exports target.

@jasnell didn't you suggest moving them to a json file?
IMHO if they are moving out they should go with their messages, and maybe after all migrations are done.

@starkwang starkwang closed this Jul 24, 2017
@jasnell
Copy link
Member

jasnell commented Jul 24, 2017

Eventually, yes, but these are fairly difficult to manage as it is. Once the migration is done we can revisit. I have a few things in mind already also...

@starkwang starkwang mentioned this pull request Aug 9, 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. doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants