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

Export styles from ansi-styles #567

Merged

Conversation

LitoMore
Copy link
Contributor

@LitoMore LitoMore commented Oct 5, 2022

ansi-styles adds some extra keys to styles:

styles.color.ansi = wrapAnsi16();
styles.color.ansi256 = wrapAnsi256();
styles.color.ansi16m = wrapAnsi16m();
styles.bgColor.ansi = wrapAnsi16(ANSI_BACKGROUND_OFFSET);
styles.bgColor.ansi256 = wrapAnsi256(ANSI_BACKGROUND_OFFSET);
styles.bgColor.ansi16m = wrapAnsi16m(ANSI_BACKGROUND_OFFSET);

We should filter them out.

Or we could process those style names in ansi-sytle, see chalk/ansi-styles#82. I marked that PR as a draft to discuss how we can improve on this.

My thoughts are:

Option 1:
Filter style names in Chalk

Option 2:
Apply chalk/ansi-styles#82, then export style names from ansi-styles in Chalk

Option 3:
Apply chalk/ansi-styles#82 change but in favor of modify the vendor/ansi-styles directly, then export styles names

What do you think?

For the exposed style names, we'd better add some tests to ensure its outputs are correct.

@sindresorhus
Copy link
Member

I would go with option 2.

Option 3 is not entirely clear to me.

@LitoMore
Copy link
Contributor Author

LitoMore commented Oct 9, 2022

Option 3 is not entirely clear to me.

I mean change the code directly in https://github.com/chalk/chalk/blob/main/source/vendor/ansi-styles/index.js. But I don't think it's a good solution.

@sindresorhus
Copy link
Member

I mean change the code directly in https://github.com/chalk/chalk/blob/main/source/vendor/ansi-styles/index.js. But I don't think it's a good solution.

No, we shouldn't do that. The vendor stuff is meant to be kept in sync with the source.

@LitoMore
Copy link
Contributor Author

LitoMore commented Oct 9, 2022

Of course. I marked the chalk/ansi-styles#82 as ready, please review.

I will sync those changes to this project.

@LitoMore LitoMore force-pushed the remove-non-styles-from-exposed-style-names branch from bc60dcf to 629d4e4 Compare October 9, 2022 16:34
@codecov
Copy link

codecov bot commented Oct 9, 2022

Codecov Report

Merging #567 (5a3245c) into main (92c55db) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #567   +/-   ##
=======================================
  Coverage   99.60%   99.61%           
=======================================
  Files           8        8           
  Lines         509      517    +8     
=======================================
+ Hits          507      515    +8     
  Misses          2        2           
Impacted Files Coverage Δ
source/index.js 99.11% <100.00%> (+0.03%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@LitoMore LitoMore changed the title Drop non-styles from exposed style names Export styles from ansi-styles Oct 9, 2022
@sindresorhus
Copy link
Member

Can you change the exports to use the Name suffix like ansi-styles? We already shipped the current names, so we need to preserve those using an alias with a @deprecated doc comment.

@LitoMore
Copy link
Contributor Author

@sindresorhus Done. This failing CI doesn't seem to be related to the code.

@Qix-
Copy link
Member

Qix- commented Oct 11, 2022

https://github.com/chalk/chalk/blob/main/.github/workflows/main.yml#L25

Not sure the goal here with this check but seems like codecov is completely broken regardless of version, it's just that it's only enabled on 16.

@LitoMore LitoMore force-pushed the remove-non-styles-from-exposed-style-names branch from 202e797 to 8011f5f Compare October 11, 2022 15:48
@LitoMore
Copy link
Contributor Author

Passing now.

@Qix-
Copy link
Member

Qix- commented Oct 11, 2022

Curious, what was the issue? The error message led me to believe it was a transient issue...

@LitoMore
Copy link
Contributor Author

No idea. I just git commit --amend my commit. I believe it's a Codecov-related issue. Maybe they fixed the problem in a short time.

source/index.d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 6e0df05 into chalk:main Oct 12, 2022
@LitoMore LitoMore deleted the remove-non-styles-from-exposed-style-names branch October 12, 2022 11:27
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.

None yet

3 participants