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

util: moving internal helpers to internal module #2025

Closed
wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

  1. Moving _errnoException and _exceptionWithHostPort to
    internal/util module as they are internal helper functions. They
    should not be exposed as part of the util module.
  2. Issuing a deprecation warning when those functions are used.

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Jun 20, 2015
@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 20, 2015
@@ -34,7 +35,7 @@ const O_WRONLY = constants.O_WRONLY || 0;
const isWindows = process.platform === 'win32';

const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
const errnoException = util._errnoException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this break graceful-fs again?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would. It's probably better to add a test for this to prevent accidental breakage

@vkurchatkin
Copy link
Contributor

@cjihrig why semver-major?

@@ -40,3 +42,42 @@ exports.deprecate = function(fn, msg) {

return deprecated;
};

exports._errnoException = function(err, syscall, original) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is going into an internal module, it doesn't need the underscore name.

Also, can we name the function (exports.errnoException = function errnoException(..) {) so we don't have to deal with exports.errnoException() below?

@brendanashworth
Copy link
Contributor

I'm going to relabel this as semver-minor as backwards compatibility is kept.

@brendanashworth brendanashworth added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 21, 2015
1. Moving `_errnoException` and `_exceptionWithHostPort` to
   `internal/util` module as they are internal helper functions. They
   should not be exposed as part of the `util` module.

2. Issuing a deprecation warning when those functions are used.
e.errno = errname;
e.syscall = syscall;
return e;
};
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this useful for making errors from libuv bindings in userland?

@ChALkeR
Copy link
Member

ChALkeR commented Jun 21, 2015

There are other modules this might break. But it's probably their fault.
For example, karma-assert sets util._errnoException for some reason:

k/karma-assert-1.0.0.tgz/assert.js:815:util._errnoException = function(err, syscall, original) {

@thefourtheye
Copy link
Contributor Author

Closing this as it breaks graceful-fs and userland functions might make errors from libuv bindings with _errnoException helper.

@thefourtheye thefourtheye deleted the util-refactoring branch June 21, 2015 03:46
@ChALkeR ChALkeR mentioned this pull request Feb 5, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Aug 17, 2016

@thefourtheye Perhaps it's time to reopen and revisit, as #6413 is going to be landed soon?

We would probably need a deprecation message on util._errnoException, though.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 29, 2016

@thefourtheye, #6413 landed, nothing is blocking this anymore. Do you wish to redo/reopen this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants