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

Add support for chalk/stderr #354

Closed
wants to merge 1 commit into from
Closed

Add support for chalk/stderr #354

wants to merge 1 commit into from

Conversation

Qix-
Copy link
Member

@Qix- Qix- commented Jul 12, 2019

Closes #301.

The diff is annoyingly misleading so here's exactly what happened:

  1. Remove supports-color from source/index.js in lieu of a generic factory function
  2. Remove the unnecessary Chalk function and references to Chalk.prototype (a regular object works fine instead).
  3. Export simply chalkFactory in source/index.js instead of the nicely wrapped chalk` instance
  4. Rename source/index.js to source/factory.js, which becomes the basis for both stderr and stdout instances
  5. Create source/index.js and stderr.js, both of which are small glue wrappers around the factory function (virtually the identical code removed from the now-renamed source/index.js) that also pass their respective supports-color objects.

Caveat 1: npm

There's no seemingly elegant way to be able to have require('chalk/stderr') pull from source/stderr.js so I pulled stderr.js into the root. It might make sense to also pull index.js into the root for consistency, which would allow us to remove main from package.json. It might also make sense to rename source -> lib in that case, too.

Seems like a giant step back but unfortunately npm is very limiting when it comes to this kind of thing.

Caveat 2: Typescript

Typescript type definition files are a nightmare, I couldn't figure out how to get typescript to recognize them. Perhaps @blakeembrey can help - as is, types do not import correctly using a tsserver instance even before these changes (on master) so something was broken beforehand.

Unless I'm mistaken, only an stderr.d.ts would have to be created with a ///<reference path="./index.d.ts" /> at the very top since the returned "types" from both index.js and stderr.js are the same.

However, none of that seems to be working. I have no idea how to debug the issue, so I'm keeping this PR strictly changes to the functional code. We should investigate typescript types before releasing chalk 3.

@Qix- Qix- requested a review from sindresorhus July 12, 2019 17:45
@blakeembrey
Copy link

@Qix- Do you want to check in what you did for stderr.d.ts or would you like me to take a pass at it in a PR to this PR? You don't actually want any reference to it, but you do need to import and re-export the types you want to use from index.d.ts. You could also put those in a third file depending on preferences.

@Qix-
Copy link
Member Author

Qix- commented Jul 13, 2019

@blakeembrey A PR would be nice, though I'm still confused as to why tsserver doesn't pick the types up. IIRC (not in front of a machine at the moment) even tsc was unable to find the types.

@sindresorhus
Copy link
Member

@Qix- This is not what we decided on in #317 (comment)

@Qix-
Copy link
Member Author

Qix- commented Aug 27, 2019

You're right, not sure how I got my signals crossed. I can try to fix it up but feel free to merge the other one if it works better.

TBPH I like this approach better. stderr being part of the API feels like pollution. But I don't feel strongly about it.

@sindresorhus
Copy link
Member

Closing in favor of #359.

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.

Auto-detect support for stderr
3 participants