events: migrate to internal/errors by jasnell · Pull Request #15623 · nodejs/node · GitHub

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

ping @nodejs/tsc

Left a few nits. LGTM if CI is green.

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

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

CI: https://ci.nodejs.org/job/node-test-pull-request/10271/

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.

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.

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.

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.

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).

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 is really the only place that we raise such errors that I can find.

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

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

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

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").

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

This type is not used anymore and should be removed.

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

@BridgeAR .. updated.
@nodejs/tsc PTAL

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

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

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.

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

LGTM

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

CI: https://ci.nodejs.org/job/node-test-pull-request/10571/

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

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.

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

@mcollina thanks for the ping

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>

Landed in e5ad545

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>

jasnell added errors Issues and PRs related to JavaScript errors originated in Node.js core. events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 26, 2017

jasnell requested review from addaleax, fhinkel, mcollina and mhdawson September 26, 2017 04:45

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. events Issues and PRs related to the events subsystem / EventEmitter. labels Sep 26, 2017

joyeecheung approved these changes Sep 26, 2017

View reviewed changes

BridgeAR reviewed Sep 26, 2017

View reviewed changes

BridgeAR requested changes Sep 28, 2017

View reviewed changes

jasnell mentioned this pull request Sep 28, 2017

Tracking Issue: Migrate errors to internal/errors.js #11273

Closed

80 tasks

jasnell force-pushed the events-internal-errors branch from 1288134 to fd7b3cc Compare October 2, 2017 19:58

jasnell requested a review from a team October 2, 2017 19:58

mhdawson approved these changes Oct 3, 2017

View reviewed changes

mcollina reviewed Oct 6, 2017

View reviewed changes

mcollina approved these changes Oct 10, 2017

View reviewed changes

joyeecheung approved these changes Oct 10, 2017

View reviewed changes

BridgeAR approved these changes Oct 10, 2017

View reviewed changes

events: migrate to internal/errors

006d963

jasnell force-pushed the events-internal-errors branch from fd7b3cc to 006d963 Compare October 13, 2017 21:15

jasnell closed this Oct 13, 2017

starkwang mentioned this pull request Oct 15, 2017

lib: move duplicate spliceOne into internal/util #16221

Closed

2 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

events: migrate to internal/errors by jasnell · Pull Request #15623 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: migrate to internal/errors - #15623

events: migrate to internal/errors#15623jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:events-internal-errorsjasnell/node:events-internal-errorsCopy head branch name to clipboard

Conversation

jasnell commented Sep 26, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Sep 26, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 2, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Oct 10, 2017

Uh oh!

joyeecheung commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Oct 10, 2017

Uh oh!

jasnell commented Oct 13, 2017

Uh oh!

events: migrate to internal/errors#15623
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:events-internal-errors

joyeecheung commented Sep 26, 2017 •
edited
Loading

mcollina commented Oct 10, 2017 •
edited
Loading