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

fix: do not show warnings when workers >= 10 #342

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

simoneb
Copy link
Contributor

@simoneb simoneb commented Mar 3, 2021

Replaces ora with char-spinner to avoid the warning altogether.

old explanation

I didn't actually figure out the root cause of why this is happening, and I do realize that making this initialization eager is doing something that's probably unnecessary unless workers are being used. Nevertheless, that's the most reliable way I found to fix this.

The problem seems to come from the bl package used by ora, which does some fancy stream manipulation, eventually leading to the warnings being generated.

Making the initialization of ora eager in the module root makes the issue disappear.

Closes #337

@simoneb simoneb force-pushed the fix/337 branch 2 times, most recently from cf6a8a1 to 613b1da Compare March 3, 2021 11:10
lib/progressTracker.js Outdated Show resolved Hide resolved
@simoneb simoneb force-pushed the fix/337 branch 3 times, most recently from 7f07651 to 63c073a Compare March 3, 2021 11:29
@@ -34,7 +35,6 @@ function track (instance, opts) {
let durationProgressBar
let amountProgressBar
let addedListeners = false
let spinner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

even if I move the instantiation here, it will still cause the test failure. moving it outside this method is what fixes it

@simoneb simoneb force-pushed the fix/337 branch 2 times, most recently from 2213afc to 2d17ef0 Compare March 3, 2021 11:39
@salmanm
Copy link
Collaborator

salmanm commented Mar 3, 2021

I think the problem starts when Ora tries to mute process.stdout stream. And process.stdout._events.xxx already contains n (number of active workers) listeners (gets added right after the call to new Worker).
So I appreciate that it's not straightforward.

But why don't we simply get rid of Ora!? I know it's widely used but if it's causing problems, I think it's worth looking for alternatives. char-spinner for instance, it's very old and not actively maintained, but I think it works fine and a very simply code base. I'd say we start using it, and if we need to tweak, we can just fork it.

@simoneb
Copy link
Contributor Author

simoneb commented Mar 3, 2021

@salmanm fair point. I changed the implementation. rationale was, I'd spent quite some time looking into the issue and the solution seemed good enough. char-spinner is much simpler it turns out so it's a more robust solution indeed. I'd assumed there was some thought put into choosing the spinner library so I didn't consider reverting that decision

@simoneb simoneb requested a review from mcollina March 3, 2021 14:25
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 267f097 into mcollina:master Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting workers to >= 10 yields eventemitter warnings
3 participants