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

feat: no longer use wrapped process.stdout and process.stderr #832

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Oct 16, 2023

When a spinner is started, it creates a mock version of process.stdout.write and process.stderr.write to capture all log messages (from console.log, debug, ux.log, etc...) and then flushes the captured logs just before re-rendering the spinner.

By encapsulating process.stdout and process.stderr (introduced here), the spinner was no longer able to capture all logs messages - just the ones that went through the encapsulated process.stdout and process.stderr. This means that anything sent to the terminal via console.log or debug was at risk of being overwritten when the spinner re-render.

The solution here is to back out of using the encapsulated process.stdout and process.stderr. These were originally added so that it would be easier to stub process.stdout|stderr.write in unit tests (see mocha issue related to stubbing process.stdout.write). In place of that, I've reduced the usage of process.stdout.write to write.stdout and write.stderr and added makeStubs method to ux that people can use if they need to stub those.

I've also deprecated the Stream class that was used to encapsulate process.stdout and process.stderr and will be removed in the next major version

Fixes #799
Closes #828
(Passing tests depend on #836)

@mdonnalley mdonnalley force-pushed the mdonnalley/no-wrapped-process branch from a45bcf8 to efae493 Compare October 16, 2023 15:03
@mdonnalley mdonnalley force-pushed the mdonnalley/no-wrapped-process branch from efae493 to 3ff5f63 Compare October 16, 2023 17:21
WillieRuemmele
WillieRuemmele previously approved these changes Oct 18, 2023
@WillieRuemmele
Copy link
Contributor

QA Notes


linked this into the demo plugin used in the issues repo:

before

➜  plugin-demo git:(main) ✗ 
 ➜  ./bin/dev demo
hello world 1
spinner... ⣻
hello world 2
spinner... done

after

➜  plugin-demo git:(main) ✗ 🔗1
 ➜  ./bin/dev demo                                                                    
hello world 1
hello world 2
hello world 3
spinner... done

linked into plugin-source doing a force source pull with a remote change

before

 ➜  sf force source pull         
Warning: We plan to deprecate this command in the future. Try using the "project retrieve start" command instead.
Loading source tracking information... ⣟
Loading source tracking information... ⣽ Retrieving metadata from the org
Loading source tracking information... ⡿ Checking for deletes from the org and updating source tracking files
Updating source tracking...
Updating source tracking... done

after

 ➜  sf force source pull
Warning: We plan to deprecate this command in the future. Try using the "project retrieve start" command instead.
Loading source tracking information... ⣷
Updating source tracking... done

@mdonnalley mdonnalley merged commit 008f1a8 into main Oct 19, 2023
34 checks passed
@mdonnalley mdonnalley deleted the mdonnalley/no-wrapped-process branch October 19, 2023 20:01
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.

ux.action.spinner overwrites most recent terminal output when updating spinner
2 participants