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

fix: replace colors with ansi-colors #3743

Closed
wants to merge 1 commit into from

Conversation

alan-agius4
Copy link
Contributor

@devoto13
Copy link
Collaborator

Unfortunately, this may be breaking to the plugins relying on the modified string prototype. I remember looking into switching to colors/safe some time ago and had to stop because of this. I don't remember any details though 😓

IMO We should probably stick with pinning for now and replace the library as part of the major release.

@alan-agius4
Copy link
Contributor Author

Would be great if you could remember the problem that it caused.

I am good in replacing in package in a major if you think it’s breaking.

@devoto13
Copy link
Collaborator

Oh, sorry! I had a tab with the PR opened since earlier and looked at the old code (replacing ''.cyan to cyan('')). Looks like the migration from colors to colors/safe has indeed happened, so this should not be a concern anymore 😄

IIRC There were some third-party plugins relying on ''.cyan in their code without explicitly importing colors, which broke when switching to the side-effect free colors/safe.

@alan-agius4
Copy link
Contributor Author

Great 🚀

@djphan
Copy link

djphan commented Jan 10, 2022

Just curious if this should be pinned to an explicit version instead of using the ^ or ~.

Just doing some dives into the colors issue and being pinned to 1.4.0 before the breaking changes would have added time. But don't want to delay any fixes

@rumbcam
Copy link

rumbcam commented Jan 10, 2022

The safest solution would be to just set colors to version 1.4.0 and drop the ^
"colors": "1.4.0",

Especially for getting a fix pushed as soon as possible. Then you can switch to ansi-colors when you have time to do some testing

@ellyxc
Copy link

ellyxc commented Jan 11, 2022

i guess few reporters like spec reporter also use colors

@jginsburgn
Copy link
Member

#3503

@jginsburgn
Copy link
Member

Due to the potential breakages I will mark this PR a WIP.

@jginsburgn jginsburgn marked this pull request as draft January 11, 2022 02:38
@DABH
Copy link
Contributor

DABH commented Feb 17, 2022

This PR can be closed in favor of #3763 . colors has been migrated to @colors/colors, so we can do the simple renaming without making any further (and in particular, no breaking) changes.

@DABH
Copy link
Contributor

DABH commented Feb 28, 2022

(As an update -- looks like #3763 was successfully merged, so I think this can be closed.)

@jginsburgn
Copy link
Member

(As an update -- looks like #3763 was successfully merged, so I think this can be closed.)

Agreed!

@jginsburgn jginsburgn closed this Feb 28, 2022
@alan-agius4 alan-agius4 deleted the ansi-colors branch February 28, 2022 19: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.

pin colors lib to 1.4.0 as it been hacked and version 1.4.1 is broken
7 participants