Skip to content

tty: expose hasColor and getColorDepth directly #40240

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thunder-coding
Copy link
Contributor

@thunder-coding thunder-coding commented Sep 28, 2021

TODOs:-

  • Expose hasColor
  • Expose getColorDepth
  • Document new methods
    - [x] Deprecate WriteStream.hasColor and WriteStream.getColorDepth
    - [x] Add tests for same
    - [ ] Replace every instance of WriteStream.hasColor and WriteStream.getColorPath with new methods in tty
    - [ ] Document new deprecated methods of tty and add documentation for new methods

About deprecating tty.WriteStream.hasColor and tty.WriteStream.getColorDepth, this requires a lot of changes to be done which is not so backward compactible.

process.stdout and process.stderr have hasColor() and getColorDepth() methods which are forwarded from tty.WriteStream, this means that deprecating tty.WriteStream.hasColor() will also mean deprecation of process.stdout.hasColor, similar for process.stdout and getColorDepth)

So I am dropping the idea of deprecating these two methods for now. I would really appreciate some feedback from the team regarding this.

Fixes #40236

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tty Issues and PRs related to the tty subsystem. labels Sep 28, 2021
@thunder-coding thunder-coding changed the title lib/tty.js: expose hasColor and getColorDepth directly tty: expose hasColor and getColorDepth directly Sep 28, 2021
@thunder-coding
Copy link
Contributor Author

I feel that a good amount of people agree with the idea of deprecating process.{stdout,stderr}.hasColor() and ...getColorDepth() (3 upvotes), if the nodejs team has no problems with this, I will go for it. I can't decide what to do until someone from the team gives their say on this

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

@thunder-coding, thanks for the PR. I definitely see value in exposing this to help terminal color library authors write smaller modules and would like to see this happen, but this PR still seems to be missing quite a few things.

Maybe if you added some documentation to this PR (and your commit message adhered to our commit message guidelines this might be able to get some traction, but until then, this is unlikely to get approval.

With regards to deprecating things, the option of documentation-deprecating something so as to not break pre-existing uses is an option that would be available if necessary. Let me know if you get stuck or need help with anything if you intend to continue pursuing this.

@thunder-coding thunder-coding marked this pull request as draft October 22, 2021 01:39
@thunder-coding thunder-coding force-pushed the lib/tty-expose-directly branch from 69b6d1d to 57223a8 Compare October 22, 2021 01:40
@thunder-coding
Copy link
Contributor Author

thunder-coding commented Oct 22, 2021

Let me know if you get stuck or need help with anything if you intend to continue pursuing this.

I have rebased this against master and even fixed the commit message. About documentation-deprecating tty.WriteStream.hasColors(), tty.WriteStream.getColorDepth() and process..... stuff, I need some help regarding on how the deprecations should be documented since this seems to be a huge change and also affects process.stdout and process.stderr

Note: I only need help regarding how to write the deprecation thing in doc/api/deprecations.md, I can manage to write in doc/api/tty.md by seeing previous deprecations

@UltiRequiem
Copy link
Contributor

Hey, I'm interested in seeing progress on this PR. Any way to help?

@thunder-coding
Copy link
Contributor Author

Hey, I'm interested in seeing progress on this PR. Any way to help?

Thanks for showing interest in my PR. I would like to continue and apply the requested changes and get this merged. But I've got quite busy nowadays due to my exams, and I won't be able to get back to this for atleast the next 2 months!

So, if anyone would like to take over the opportunity to create a new PR which implements it properly is free to do so!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import {getColorDepth} from 'tty'
4 participants