search

dlmanning / gulp-sass

SASS plugin for gulp
MIT License
1.56k stars 381 forks source link

Replace chalk & transfob dev dependencies #828

Closed mxmason closed 2 years ago

mxmason commented 2 years ago

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

XhmikosR commented 2 years ago

I'd go with picocolors :)

On Mon, Oct 25, 2021, 08:03 EJ Mason @.***> wrote:

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

  • colorette https://github.com/jorgebucaran/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

You can view, comment on, or merge this pull request online at:

https://github.com/dlmanning/gulp-sass/pull/828 Commit Summary

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dlmanning/gulp-sass/pull/828, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVLNMTDJ7FF6NWCSFTETDUITQJNANCNFSM5GUK2DYQ .

XhmikosR commented 2 years ago

Regarding transfob, I did the same at some point but decided to drop the patch in favor of not risking something to break (although the patch looks OK from a quick look).

On Mon, Oct 25, 2021, 08:03 EJ Mason @.***> wrote:

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

  • colorette https://github.com/jorgebucaran/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

You can view, comment on, or merge this pull request online at:

https://github.com/dlmanning/gulp-sass/pull/828 Commit Summary

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dlmanning/gulp-sass/pull/828, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVLNMTDJ7FF6NWCSFTETDUITQJNANCNFSM5GUK2DYQ .

XhmikosR commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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? https://github.com/dlmanning/gulp-sass/compare/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 commented 2 years ago

@xzyfer can you please check out https://github.com/dlmanning/gulp-sass/compare/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).