Skip to content

Conversation

@targos
Copy link
Member

@targos targos commented Jul 3, 2021

This reverts commit 954217a.

It breaks npm.

@github-actions github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 3, 2021
@targos
Copy link
Member Author

targos commented Jul 3, 2021

To reproduce the issue before this commit:

rm -rf tools/doc/node_modules
make doc

@targos
Copy link
Member Author

targos commented Jul 3, 2021

Here's the related code in npm that doesn't work as expected with the change:

https://github.com/npm/gauge/blob/e1839fb2fddb15b6e468ff5ec90086c3c59e084d/index.js#L226-L232

It seems that either the write method now returns false instead of true, or the 'drain' event is no longer emitted.
_writeTo is the process.stderr stream.

@ronag
Copy link
Member

ronag commented Jul 3, 2021

I’ll take a look tonight

@ronag
Copy link
Member

ronag commented Jul 3, 2021

@targos any possibility of a repro using just gauge?

@targos
Copy link
Member Author

targos commented Jul 3, 2021

@ronag

var Gauge = require('./deps/npm/node_modules/gauge/index.js');

var gauge = new Gauge(process.stderr, {
  theme: {hasColor: true},
  template: [
    {type: 'progressbar', length: 20},
    {type: 'activityIndicator', kerning: 1, length: 1},
    {type: 'section', default: ''},
    ':',
    {type: 'logline', kerning: 1, default: ''}
  ]
})

gauge.show("working…", 0)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.25) }, 500)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.50) }, 1000)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.75) }, 1500)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.99) }, 2000)
setTimeout(() => gauge.hide(), 2300)

@ronag
Copy link
Member

ronag commented Jul 3, 2021

There is a redraw live loop bug in gauge… I’m not sure what change triggers it. It unnecessarily calls redraw in drain. I’ll try and see if I can workaround it without changing gauge.

ronag added a commit to nxtedition/node that referenced this pull request Jul 3, 2021
HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.

Refs: nodejs#39246
@ronag ronag mentioned this pull request Jul 3, 2021
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Fixed in #39253

@targos targos closed this Jul 5, 2021
ronag added a commit that referenced this pull request Jul 7, 2021
HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.

Refs: #39246

PR-URL: #39253
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targos targos deleted the revert-34385 branch July 25, 2021 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants