FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util(styleText): optimise + benchmark by AugustinMauroy · Pull Request #58063 · nodejs/node · GitHub

/ node Public

util(styleText): optimise + benchmark - #58063

Closed
AugustinMauroy wants to merge 3 commits into
nodejs:mainfrom
AugustinMauroy:pref(util)-optimise-styleText
ConversationCommits (3)ChecksFiles changed
Closed

util(styleText): optimise + benchmark#58063
AugustinMauroy wants to merge 3 commits into
nodejs:mainfrom
AugustinMauroy:pref(util)-optimise-styleText

Conversation

AugustinMauroy commented Apr 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

I had modify logic behind. using a reducer, it's should be quicker. And I had updated benchmark

Plus I had updated test to use node:test in goal of having better DX when something fails.

nodejs-github-bot commented Apr 28, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 28, 2025

codecov Bot commented Apr 28, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.21%. Comparing base (6cd1c09) to head (a9d13f2).
⚠️ Report is 2518 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #58063   +/-   ##
=======================================
  Coverage   90.21%   90.21%           
=======================================
  Files         630      630           
  Lines      186391   186448   +57     
  Branches    36610    36620   +10     
=======================================
+ Hits       168146   168209   +63     
+ Misses      11066    11038   -28     
- Partials     7179     7201   +22
Files with missing lines Coverage Δ
lib/util.js 97.77% <100.00%> (+0.01%) ⬆️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lpinca commented Apr 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Plus I had updated test to use node:test in goal of having better DX when something fails.

Please don't do that, we discussed it a lot. Those refactors are not useful.

AugustinMauroy commented Apr 28, 2025

Copy link
Copy Markdown
Member Author

Please don't do that, we discussed it a lot. Those refactors are not useful.

oh okay, surprising because the use cases with the it allow you to find your way around the test and, in the event of a failure, search the codebase.
Do you have a link to the discussion?

lpinca commented Apr 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Start here #54796 and here #56027, follow the links there.

AugustinMauroy force-pushed the pref(util)-optimise-styleText branch 2 times, most recently from 7e02fbd to 369b13f Compare April 29, 2025 09:34
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment thread test/parallel/test-util-styletext.js Outdated
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment thread test/parallel/test-util-styletext.js
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment thread test/parallel/test-util-styletext.js
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment thread test/parallel/test-util-styletext.js Outdated
AugustinMauroy force-pushed the pref(util)-optimise-styleText branch from cf38ccd to ae69a04 Compare April 29, 2025 09:47
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment thread benchmark/util/style-text.js Outdated
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment thread benchmark/util/style-text.js Outdated
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment thread benchmark/util/style-text.js Outdated
marco-ippolito reviewed Apr 29, 2025
View reviewed changes
Comment thread lib/util.js Outdated
@@ -129,8 +128,9 @@ function styleText(format, text, { validateStream = true, stream = process.stdou
throw new ERR_INVALID_ARG_TYPE('stream', ['ReadableStream', 'WritableStream', 'Stream'], stream);
}

// If the stream is falsy or should not be colorized, set skipColorize to true
skipColorize = !lazyUtilColors().shouldColorize(stream);
if (!lazyUtilColors().shouldColorize(stream)) {

marco-ippolito Apr 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

early return will skip validation so its a necessary overhead
#56722 (comment)

RafaelGSS requested changes Apr 29, 2025
View reviewed changes

RafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As Marco mentioned, the "overhead" is necessary.

AugustinMauroy commented Apr 29, 2025

Copy link
Copy Markdown
Member Author

Can I just keep benchmark change ?

RafaelGSS commented Apr 29, 2025

Copy link
Copy Markdown
Member

Can I just keep benchmark change ?

Yes, but I'd do it as a separate benchmark, so we don't need to have benchmarks for:

noColors: true, format: 'italic',
noColors: true, format: 'underline',
noColors: true, format: 'read',

As they will all be the same. So, adding it as a separate benchmark makes more sense.

util(styleText): optimise + benchmark
ef34ff0 
Co-Authored-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
AugustinMauroy force-pushed the pref(util)-optimise-styleText branch from 029701a to ef34ff0 Compare April 30, 2025 10:14
AugustinMauroy changed the title perf(util): optimise styleText util(styleText): optimise + benchmark Apr 30, 2025
RafaelGSS requested changes Apr 30, 2025
View reviewed changes
Comment thread benchmark/util/style-text.js
validateStream: [1, 0],
n: [1e3],
});
withColor: {

RafaelGSS Apr 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please, create a different file instead. style-text-nocolor.js

AugustinMauroy Apr 30, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

oh okay got it !

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

github-actions Bot added stale and removed stale labels Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 left review comments
marco-ippolito marco-ippolito left review comments
RafaelGSS RafaelGSS requested changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL