Skip to content
This repository has been archived by the owner on May 4, 2024. It is now read-only.

Allow 'error' prefix #19

Closed
wants to merge 1 commit into from
Closed

Allow 'error' prefix #19

wants to merge 1 commit into from

Conversation

bengl
Copy link
Contributor

@bengl bengl commented Jul 22, 2015

Prior to this commit, any time you'd log something with 'error' as the prefix, it would give something like:

$ node -e "require('npmlog').error('error', 'some kind of error message')"

events.js:87
      throw Error('Uncaught, unspecified "error" event.');
            ^
Error: Uncaught, unspecified "error" event.
    at Error (native)
    at EventEmitter.emit (events.js:87:13)
    at EventEmitter.<anonymous> ($PWD/node_modules/npmlog/log.js:151:22)
    at EventEmitter.<anonymous> ($PWD/node_modules/npmlog/log.js:223:21)
    at [eval]:1:19
    at Object.exports.runInThisContext (vm.js:74:17)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:460:26)
    at evalScript (node.js:431:25)
    at startup (node.js:90:7)

This is because when there's a prefix, an event is emitted with the prefix as its type, and 'error'-type events are treated in a special way.

The quick and easy fix is to add a dummy event listener for 'error', so that's what I've done here. Plus a test.

The (reasonable, I think) side effect here is that it that emitted errors won't get thrown on their own.

@gr2m
Copy link

gr2m commented Oct 30, 2015

We've run into the same problem. Is there a reason why we can't merge this besides the failing tests?

@othiym23 othiym23 self-assigned this Oct 30, 2015
@othiym23
Copy link
Contributor

This is probably fine to land, but it's going to be a semver-major change (which is no problem for npm, because same maintainers etc) because it's possible somebody was relying on the old behavior. For some reason. 🎉

@gr2m
Copy link

gr2m commented Oct 30, 2015

This is probably fine to land, but it's going to be a semver-major change (which is no problem for npm, because same maintainers etc) because it's possible somebody was relying on the old behavior. For some reason. 🎉

I love this very much 💐 💞 #teamSemanticRelease #teamGreenkeeper

@othiym23
Copy link
Contributor

othiym23 commented Nov 5, 2015

Landed as d6b2162, will be released as npmlog@2 as soon as Travis goes green. Thanks, Bryan!

@othiym23 othiym23 closed this Nov 5, 2015
@bengl
Copy link
Contributor Author

bengl commented Nov 5, 2015

🎅

othiym23 added a commit to npm/npm that referenced this pull request Nov 5, 2015
Override default EventEmitter error-handling behavior. Some people might
want to log messages with 'error' as the name. Only semver-major because
there might have been somebody relying on the fact that npmlog was an
EE.

Credit: @bengl
PR-URL: npm/npmlog#19
othiym23 added a commit to npm/npm that referenced this pull request Nov 5, 2015
Override default EventEmitter error-handling behavior. Some people might want
to log messages with 'error' as the name. Only semver-major because there might
have been somebody relying on the fact that npmlog was an EE.

Credit: @bengl
PR-URL: npm/npmlog#19
@gr2m
Copy link

gr2m commented Nov 5, 2015

yay 😃🐼

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants