GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — events: make sure console functions exist by davidvgalbraith · Pull Request #4479 · nodejs/node

Conversation

@davidvgalbraith

@davidvgalbraith davidvgalbraith commented Dec 30, 2015

Copy link
Copy Markdown

If there's no global console cached, these calls will initialize it,
which in some cases can cause a circular dependency, making the console
received here an empty object. The program shouldn't crash in this case.

Fixes #4467

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dave
events: make sure console functions exist
362d832 
if there's no global console cached, these calls will initialize it,
which in some cases can cause a circular dependency, making the console
received here an empty object. The program shouldn't crash in this case.

Fixes nodejs#4467
@davidvgalbraith davidvgalbraith mentioned this pull request Dec 30, 2015
console.error is not a function #4467
Closed
@JungMinu JungMinu added the events Issues and PRs related to EventEmitter and the events module. label Dec 30, 2015
@davidvgalbraith

davidvgalbraith commented Dec 30, 2015

Copy link
Copy Markdown
Author

A more general fix would be to always eagerly cache the global console object. I am not sure what the startup performance tradeoff equation is with that, so I went with this minimal fix for the issue at hand.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JungMinu JungMinu added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. util Issues and PRs related to the built-in util module. and removed lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Dec 30, 2015
@jasnell

jasnell commented Dec 30, 2015

Copy link
Copy Markdown
Member

@nodejs/ctc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mscdex
mscdex reviewed Dec 30, 2015
View reviewed changes
Comment thread test/parallel/test-global-console-exists.js Outdated

@mscdex mscdex Dec 30, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Perhaps it would be better to use a public API to check, such as:

assert.ok(!e.listeners('hello')[0].hasOwnProperty('warned'));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@davidvgalbraith davidvgalbraith Dec 30, 2015

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The listeners() API leaves out the warned property: https://github.com/nodejs/node/blob/master/lib/events.js#L393.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Jan 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

While I agree that it's the most efficient approach, using an internal API in a test is a bit troublesome. The "correct" way to do this would likely be to direct the stderr/stdout output to a stream and apply a regex to ensure that the result is as the user should expect it to be. It's a bit of a workaround but it ensures that we're testing exactly what the user would see.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dave
initialize console in defaultMaxListeners setter
9f8eddb
@davidvgalbraith
davidvgalbraith force-pushed the console-error-is-a-function branch from 3e80ddd to 9f8eddb Compare January 1, 2016 20:07
@davidvgalbraith

davidvgalbraith commented Jan 1, 2016

Copy link
Copy Markdown
Author

I thought up a better way to do this: now I include a reference to console in the setter for EventEmitter.defaultMaxListeners, so it'll always be compiled after you change the property. The advantage of this over my old approach is the old way wouldn't log the warning it got where console was uninitialized, whereas this approach always has the console initialized by the time it gets to logging the warning. It also was a strange leap of logic to have util.js and events.js accounting for a potentially uninitialized console, so I like this better.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 4, 2016

Copy link
Copy Markdown
Member

Much better approach. I always get a bit concerned when replacing bare properties with getter/setter but in this instance I cannot imagine that there'd be any issue. The actual code change LGTM but I think the test needs a bit more work to avoid using the internal API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 4, 2016
@jasnell

jasnell commented Jan 4, 2016

Copy link
Copy Markdown
Member

Marking as a semver-minor due to the switch to using a getter/setter
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Dave
test: overwrite stderr.write instead of checking e._events
04a9276
@davidvgalbraith

davidvgalbraith commented Jan 4, 2016

Copy link
Copy Markdown
Author

I refactored the test as per your suggestion @jasnell.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 8, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/1170/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 8, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
silverwind
silverwind reviewed Jan 11, 2016
View reviewed changes
Comment thread test/parallel/test-global-console-exists.js Outdated

@silverwind silverwind Jan 11, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Was this dot meant to be matched as a dot? You should escape it in that case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@silverwind

silverwind commented Jan 11, 2016

Copy link
Copy Markdown
Contributor

LGTM besides the two nits above.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Dave
global console exists spec cleanups
ff4065e 
eslint-disable required-modules
escape regex dot
@davidvgalbraith

davidvgalbraith commented Jan 11, 2016

Copy link
Copy Markdown
Author

Good ideas @silverwind, I've incorporated them.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind

silverwind commented Jan 11, 2016

Copy link
Copy Markdown
Contributor

LGTM pending CI: https://ci.nodejs.org/job/node-test-pull-request/1199/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
thefourtheye
thefourtheye reviewed Jan 11, 2016
View reviewed changes
Comment thread test/parallel/test-global-console-exists.js Outdated

@thefourtheye thefourtheye Jan 11, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Redundant EventEmitter

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dave
Use the default export EventEmitter from events
e1a14e7
@davidvgalbraith

davidvgalbraith commented Jan 12, 2016

Copy link
Copy Markdown
Author

Thanks @thefourtheye, updated.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind

silverwind commented Jan 12, 2016

Copy link
Copy Markdown
Contributor

CI likes it. Still LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
thefourtheye
thefourtheye reviewed Jan 12, 2016
View reviewed changes
Comment thread test/parallel/test-global-console-exists.js Outdated

@thefourtheye thefourtheye Jan 12, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Shouldn't we also assert if the console object is properly initialized by now?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@davidvgalbraith davidvgalbraith Jan 12, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think there's any way we can do that without incidentally initializing it. Our prior logging would have failed if console didn't initialize properly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Jan 12, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How about checking if console.error function exists by the time control reaches here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@davidvgalbraith davidvgalbraith Jan 12, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Such a reference to console.error will compile console if it isn't already compiled.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@davidvgalbraith davidvgalbraith Jan 12, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I suppose we could inspect require('native_module')._cache.console.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@davidvgalbraith davidvgalbraith Jan 12, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Scratch that -- Error: Cannot find module 'native_module'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Jan 12, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hmmm, my office time has started. I'll think about this when I find time today. Let's not block this PR because of this. Perhaps you might want to leave a TODO or something in the test for the timebeing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
thefourtheye
thefourtheye reviewed Jan 12, 2016
View reviewed changes
Comment thread test/parallel/test-global-console-exists.js

@thefourtheye thefourtheye Jan 12, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is why we have assert.equal(write_calls, 2); at the end, right?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@davidvgalbraith davidvgalbraith Jan 12, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure! I felt like I should have some sort of catch-all else statement here though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dave
TODO validate console
7152739
jasnell
jasnell reviewed Jan 15, 2016
View reviewed changes
Comment thread test/parallel/test-global-console-exists.js

@jasnell jasnell Jan 15, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why add the comment vs. adding the validation?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@davidvgalbraith davidvgalbraith Jan 15, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

As per earlier discussion (#4479 (diff)) I don't think it's actually possible to validate the console here in any significant way, but @thefourtheye requested a comment about validating it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Jan 15, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

oh :-) lol completely missed that one

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Jan 15, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell pushed a commit that referenced this pull request Jan 15, 2016
Dave @jasnell
events: make sure console functions exist
f9a59c1 
If there's no global console cached, initialize it.

Fixes: #4467
PR-URL: #4479
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell

jasnell commented Jan 15, 2016

Copy link
Copy Markdown
Member

Landed in f9a59c1...

@davidvgalbraith .. I squashed the commits and reworked the commit message to make sure it fit within our style guidelines. I also went ahead and expanded that TODO comment in the test case to make sure it describes exactly why we're leaving it as a TODO
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Jan 15, 2016
@davidvgalbraith

davidvgalbraith commented Jan 15, 2016

Copy link
Copy Markdown
Author

Thanks, @jasnell! I'm glad we got this one in.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas evanlucas mentioned this pull request Jan 18, 2016
v5.5.0 Release Proposal (Stable) #4742
Merged
evanlucas pushed a commit that referenced this pull request Jan 18, 2016
Dave @evanlucas
events: make sure console functions exist
a8330f7 
If there's no global console cached, initialize it.

Fixes: #4467
PR-URL: #4479
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas added a commit that referenced this pull request Jan 20, 2016
@evanlucas
2016-01-20, Version 5.5.0 (Stable)
dd88256 
Notable changes:

* events: make sure console functions exist (Dave) #4479
* fs: add autoClose option to fs.createWriteStream (Saquib) #3679
* http: improves expect header handling (Daniel Sellers) #4501
* node: allow preload modules with -i (Evan Lucas) #4696
* v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463
* Minor performance improvements:
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622
  - module: cache stat() results more aggressively (Ben Noordhuis) #4575
  - querystring: improve parse() performance (Brian White) #4675

PR-URL: #4742
evanlucas added a commit that referenced this pull request Jan 21, 2016
@evanlucas
2016-01-20, Version 5.5.0 (Stable)
318ded2 
Notable changes:

* events: make sure console functions exist (Dave) #4479
* fs: add autoClose option to fs.createWriteStream (Saquib) #3679
* http: improves expect header handling (Daniel Sellers) #4501
* node: allow preload modules with -i (Evan Lucas) #4696
* v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463
* Minor performance improvements:
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622
  - module: cache stat() results more aggressively (Ben Noordhuis) #4575
  - querystring: improve parse() performance (Brian White) #4675

PR-URL: #4742
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Dave Michael Scovetta
events: make sure console functions exist
8cd93bf 
If there's no global console cached, initialize it.

Fixes: nodejs#4467
PR-URL: nodejs#4479
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@evanlucas Michael Scovetta
2016-01-20, Version 5.5.0 (Stable)
de2ebef 
Notable changes:

* events: make sure console functions exist (Dave) nodejs#4479
* fs: add autoClose option to fs.createWriteStream (Saquib) nodejs#3679
* http: improves expect header handling (Daniel Sellers) nodejs#4501
* node: allow preload modules with -i (Evan Lucas) nodejs#4696
* v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) nodejs#4463
* Minor performance improvements:
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622
  - module: cache stat() results more aggressively (Ben Noordhuis) nodejs#4575
  - querystring: improve parse() performance (Brian White) nodejs#4675

PR-URL: nodejs#4742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

events Issues and PRs related to EventEmitter and the events module. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@davidvgalbraith @jasnell @silverwind @mscdex @thefourtheye @JungMinu
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.