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

events: migrate to internal/errors #15623

Closed
wants to merge 1 commit into from

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Sep 26, 2017

Migrate events.js to internal errors

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

events,errors

@jasnell
Copy link
Member Author

@jasnell jasnell commented Sep 26, 2017

Copy link
Member

@joyeecheung joyeecheung left a comment

Left a few nits. LGTM if CI is green.

doc/api/errors.md Outdated
<a id="ERR_EVENTS_MAX_LISTENERS_TYPE"></a>
### ERR_EVENTS_MAX_LISTENERS_TYPE

Used when an attempt is made to set `require('events').defaultMaxListeners` to

This comment has been minimized.

@joyeecheung

joyeecheung Sep 26, 2017
Member

Would be nice to link this to the documentation of EventEmitter.defaultMaxListeners

lib/events.js Outdated
throw new TypeError('"listener" argument must be a function');
if (typeof listener !== 'function') {
const errors = lazyErrors();
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'listener', 'Function');

This comment has been minimized.

@joyeecheung

joyeecheung Sep 26, 2017
Member

s/Function/function/? Most of the ERR_INVALID_ARG_TYPEs in the code base are using lowercased types.

lib/events.js Outdated
if (typeof listener !== 'function') {
const errors = lazyErrors();
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'listener',
'Function');

This comment has been minimized.

@BridgeAR

BridgeAR Sep 26, 2017
Member

Our docs mainly use Function but it is going to print type Function and this is somewhat inconsistent out of my perspective. I think we should stick to the lower cased version.

if (typeof listener !== 'function')
throw new TypeError('"listener" argument must be a function');
if (typeof listener !== 'function') {
const errors = lazyErrors();

This comment has been minimized.

@BridgeAR

BridgeAR Sep 26, 2017
Member

Do you know where the circular reference is coming from? If so - is there any possibility in untying that?
I personally also prefer using a simple if a lot over the function version. It is still a minor overhead of using the function while there is no faster check than a simple if to undefined.

This comment has been minimized.

@jasnell

jasnell Sep 26, 2017
Author Member

It's not a circular reference that is the concern. The events module is one of the very first modules loaded in core (because it is needed by process) and as such needs to be loaded before anything else is loaded.

lib/events.js Outdated
throw new TypeError('"defaultMaxListeners" must be a positive number');
if (typeof arg !== 'number' || arg < 0 || arg !== arg) {
const errors = lazyErrors();
throw new errors.TypeError('ERR_EVENTS_MAX_LISTENERS_TYPE');

This comment has been minimized.

@BridgeAR

BridgeAR Sep 26, 2017
Member

I do not really see the point for this new error type. I it extremely specific and the type name is somewhat confusing for me. I would expect it to describe having e.g. to many listeners. Using a more generic one that is similar to the old error message seems more appropriate to me.

This comment has been minimized.

@jasnell

jasnell Sep 26, 2017
Author Member

The challenge is that none of the existing error codes quite fit this pattern (e.g. it's a setter and not an argument or an option).

doc/api/errors.md Outdated
an invalid type.

<a id="ERR_EVENTS_UNHANDLED_ERROR"></a>
### ERR_EVENTS_UNHANDLED_ERROR

This comment has been minimized.

@BridgeAR

BridgeAR Sep 26, 2017
Member

Do we have to make this specific to the events? I would rather have this in case any unhandled error is thrown / emitted what ever.

This comment has been minimized.

@jasnell

jasnell Sep 26, 2017
Author Member

This is really the only place that we raise such errors that I can find.

This comment has been minimized.

@BridgeAR

BridgeAR Sep 28, 2017
Member

That is correct for the current situation but for me it is more about trying to be open for future uses.

lib/internal/errors.js Outdated
E('ERR_EVENTS_UNHANDLED_ERROR',
(err) => {
const msg = 'Unhandled "error" event.';
if (!err) return msg;

This comment has been minimized.

@BridgeAR

BridgeAR Sep 26, 2017
Member

Using if (err !== undefined) would be better as there might be other falsy values out there that someone throws.

test/parallel/test-event-emitter-max-listeners.js Outdated
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "n" argument must be of type positive number'

This comment has been minimized.

@BridgeAR

BridgeAR Sep 26, 2017
Member

This is a really confusing error message. I think we have a more appropriate error message for things like that.

Copy link
Member

@BridgeAR BridgeAR left a comment

The obsolete error code should be removed. LGTM otherwise (I do not feel that strongly about the more generic name even if I would still prefer it without "EVENTS").

doc/api/errors.md Outdated
an invalid type.

<a id="ERR_EVENTS_UNHANDLED_ERROR"></a>
### ERR_EVENTS_UNHANDLED_ERROR

This comment has been minimized.

@BridgeAR

BridgeAR Sep 28, 2017
Member

That is correct for the current situation but for me it is more about trying to be open for future uses.

doc/api/errors.md Outdated
@@ -644,6 +644,17 @@ according to the encoding provided.
Used by the `util.TextDecoder()` API when the encoding provided is not one of
the [WHATWG Supported Encodings][].

<a id="ERR_EVENTS_MAX_LISTENERS_TYPE"></a>
### ERR_EVENTS_MAX_LISTENERS_TYPE

This comment has been minimized.

@BridgeAR

BridgeAR Sep 28, 2017
Member

This type is not used anymore and should be removed.

This comment has been minimized.

@jasnell

jasnell Sep 28, 2017
Author Member

oh right... heh, forgot about that ;-)

@jasnell jasnell mentioned this pull request Sep 28, 2017
79 of 80 tasks complete
@jasnell jasnell force-pushed the jasnell:events-internal-errors branch Oct 2, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 2, 2017

@BridgeAR .. updated.
@nodejs/tsc PTAL

@jasnell jasnell requested a review from nodejs/tsc Oct 2, 2017
Copy link
Member

@mhdawson mhdawson left a comment

This change looks good, but I think we need to add a test for the new error in test/parallel/test-internal-errors.js

if (errors === undefined)
errors = require('internal/errors');
return errors;
}

This comment has been minimized.

@mcollina

mcollina Oct 6, 2017
Member

Why do we need to lazily load this? Is this the pattern used everywhere? Should we?

This comment has been minimized.

@jasnell

jasnell Oct 9, 2017
Author Member

It is because events.js is loaded before all the other modules in the dependency tree (because of bootstrapping process. Lazy loading errors prevents errors from being loaded before events is loaded.

This comment has been minimized.

@BridgeAR

BridgeAR Oct 10, 2017
Member

I would still rather inline this instead of using the function form but it should not be a blocker.

Copy link
Member

@mcollina mcollina left a comment

LGTM

@mcollina
Copy link
Member

@mcollina mcollina commented Oct 10, 2017

@BridgeAR are you ok with the change? Can we land?

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 10, 2017

Would be better if we can specify the range with ERR_OUT_OF_RANGE, that can be done later in another PR.

{
code: 'ERR_UNHANDLED_ERROR',
type: Error,
message: 'Unhandled error. (Accepts a string)'

This comment has been minimized.

@BridgeAR

BridgeAR Oct 10, 2017
Member

The message looks somewhat weird here because of the dot before the brackets.
I think it would be fine to either remove the dot or to move it to the end of the string.

if (errors === undefined)
errors = require('internal/errors');
return errors;
}

This comment has been minimized.

@BridgeAR

BridgeAR Oct 10, 2017
Member

I would still rather inline this instead of using the function form but it should not be a blocker.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Oct 10, 2017

@mcollina thanks for the ping

@jasnell jasnell force-pushed the jasnell:events-internal-errors branch to 006d963 Oct 13, 2017
jasnell added a commit that referenced this pull request Oct 13, 2017
PR-URL: #15623
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 13, 2017

Landed in e5ad545

@jasnell jasnell closed this Oct 13, 2017
@starkwang starkwang mentioned this pull request Oct 15, 2017
2 of 2 tasks complete
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
PR-URL: nodejs/node#15623
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.