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.

Sign up for GitHub

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

Jump to bottom

Replace chalk & transfob dev dependencies #828

Closed
wants to merge 1 commit into from
Closed

Replace chalk & transfob dev dependencies #828

wants to merge 1 commit into from

Conversation

mxmason
Copy link
Contributor

@mxmason mxmason commented Oct 25, 2021

👋🏻 I saw that @XhmikosR has been doing some cleanup work on my v5 release (thanks!) and I wanted to contribute my own housecleaning.

  • colorette is much smaller than chalk (and supports node >= 10).
  • since transfob is very small, and makes transforms by modifying the transform object instead of passing a function in the opts when it's created, I thought we could just remake it ourselves

@mxmason
Replace chalk & transfob dependencies
26f9643 
- Replaces chalk with colorette
- Replaces transfob with local `transfob` function, because external dependency created transform object in a slightly imporper way.
@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 25, 2021 via email

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 25, 2021 via email

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 25, 2021 •
edited
Loading

So, my suggestion would be to break these into two patches and perhaps switch to ${colors.underline(relativePath)}\n${error.formatted} while at it. No need to use an Array and then switch it to a string when we can use a String in the first case :)

For reference: https://github.com/XhmikosR/gulp-sass/commit/d955da1ad46d93e7f8464f8ea3127e06e59129a8

@XhmikosR
Copy link
Contributor

@xzyfer WDYT about the above? picocolors is new, but widely used too and it's much smaller/faster than chalk. colorette is also a viable solution. The reason I went with picocolors is because it's used by postcss, autoprefixer etc so chances are it'll be already in the deps and it's smaller.

@xzyfer
Copy link
Collaborator

xzyfer commented Oct 25, 2021

Thanks for your work but this project is in maintenance mode and has been materially unchanged for a couple years.

As stated on @XhmikosR's PRs aside from security and compatibility fixes we have no intention to make unnecessary changes at this time.

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 25, 2021 •
edited
Loading

I still believe we could land these patches, maybe not the transfob one since it's a small package anyway. Could we at least land the Array change and the default param change from here? master...XhmikosR:dev

Note that the ESLint consistent return seems to be valid too; it's just that gulp handles return foo() as foo(); return;.

@XhmikosR
Copy link
Contributor

@xzyfer can you please check out master...XhmikosR:dev and let me know how to proceed?

I still think it's totally worth switching to picocolors since postcss is using it, klona it's less important, and the rest of the patches should be safe except for the return statements patch (which should be OK too, but I don't want to risk it).

@XhmikosR XhmikosR mentioned this pull request Dec 7, 2021
Merged
This was referenced Dec 27, 2021
Merged
Merged
@xzyfer xzyfer closed this in #840 Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mxmason @XhmikosR @xzyfer