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

Improve the detections #98

Closed
ehmicky opened this issue Mar 29, 2019 · 7 comments
Closed

Improve the detections #98

ehmicky opened this issue Mar 29, 2019 · 7 comments
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted

Comments

@ehmicky
Copy link

ehmicky commented Mar 29, 2019

Issuehunt badges

getColorDepth() was added in Node.js 9.9.0 and hasColors() and FORCE_COLOR=0|1|2|3 to Node.js 11.13.0. They are directly based on this module's code.

supports-color brings extra features:

  • supports CLI flag [--no]-color[s]
  • supports browsers (which always default to "no colors")
  • hasColors() is undefined if stream.isTTY is not true, i.e. one needs to do stream.hasColors !== undefined && stream.hasColors()

On the other hand, getColorDepth() and hasColors():

  • are in core Node.js
  • supports more fine grained color detection logic. See the current code. They basically handle much more environment variables cases.

My question is: wouldn't it make sense to do the following?

  • export ponyfills of getColorDepth() and hasColors(). Basically, just copy/pasting the code. If the Node.js running version supports those functions, they can be directly used. Those ponyfills could be removed in the future once Node.js supported versions are upgraded.
  • use those methods instead of the current detection logic.

IssueHunt Summary

Backers (Total: $60.00)

Become a backer now!

Or submit a pull request to get the deposits!

Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Member

It's awesome that it's now in Node.js. The problem is that the detection will be ever changing, so I don't think we'll ever be able to actually use the native methods as we favor stability over all Node.js versions we support. Imagine if the detection in Node.js core changes between each major version and we support 3 major versions in Chalk. That will create a bad experience for the user.

Another reason I don't want to depend on the Node.js logic is that we can move faster while Node.js is locked into their major version schedule for breaking changes.

I think the best way forward is to just improve our detection with inspiration from the Node.js logic.


They basically handle much more environment variables cases.

That's not always a good thing. The more fine-grained cases you handle, the more assumption there are that could be broken at any time.

@sindresorhus sindresorhus changed the title Using getColorDepth() core method Improve the detections Jul 12, 2019
@sindresorhus
Copy link
Member

I'm gonna re-title this issue to be about improving the detection logic.

@issuehunt-oss
Copy link

issuehunt-oss bot commented Jul 12, 2019

@issuehunt has funded $60.00 to this issue.


@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label Jul 12, 2019
@Qix-
Copy link
Member

Qix- commented Jul 12, 2019

@sindresorhus idk about you but I'd personally love to retire the argv sniffing we do in this package.

@sindresorhus
Copy link
Member

@Qix- I don't think we should retire it, it's useful for the common case. Instead, we should also expose the detections without the flag logic, per #40 (comment).

@Qix-
Copy link
Member

Qix- commented Jul 12, 2019

@sindresorhus It causes some annoyance when dealing with argument parsing, and I personally believe it's out of scope for the project. This module is to detect color support in the terminal with a potential environment variable override. Sniffing argv is pretty sacred IMO, and while I understand that might break some applications that rely on this it wouldn't be hard to migrate whatsoever.

It might make sense to support an .setGlobalSupportLevel() method to help polyfilling the old support.

But as it stands, I think it's an artifact of feature creep since it's very intensely opinionated and non-specified. We're essentially dictating how the CLI of a program should look or behave, especially without a lot of users even knowing it.

Not that we have any real metrics to support it, but I hypothesize that the vast majority of users that want to override the color support level use environment variables instead of command line flags.

Just my $0.02. I think I left a comment elsewhere (I can't find it now) about how this would look programmatically with chalk, since it's our no.1 consumer.

@Qix-
Copy link
Member

Qix- commented Aug 27, 2019

So I've since updated my views after looking at some of the PRs that went into Node's color depth "feature".

The amount of thought and discussion that went into it was next to nothing. Their use of the term "depth" isn't even correct.

Some of the PRs were merged even after someone objected with "perhaps we should standardize this". None of us at chalk were pinged - which I realize isn't a requirement, but a lot of the concerns and discussions that would have benefitted the implementation are those we've had time and time again with users, developers and terminal emulator implementors alike over almost a decade.

That color depth method will probably break your code at some point, given how little thought and effort went into it. I'd highly recommend against using it.


While this sounds like something we should support, there's too high of a chance this would cause headache in the future. It's really hard to remove a check gracefully, without breaking a lot of people, than it is to add it - and this would definitely cause problems later on.

Thank you for the suggestion, it was a very good one but alas just didn't pan out :)

@Qix- Qix- closed this as completed Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants