lib: remove startsWith/endsWith primordials for char checks #55407

gurgunday · 2024-10-16T16:53:41Z

Removes primordials when we're only checking for one character and leans on JavaScript syntax as much as possible for performance

nodejs-github-bot · 2024-10-16T16:53:47Z

panva · 2024-10-16T16:59:16Z

test/fixtures/wpt/resources/webidl2/lib/webidl2.js

This is an upstream repository resource, please remove the change.

lib/internal/modules/esm/fetch_module.js

lib/internal/modules/esm/resolve.js

lib/internal/modules/esm/fetch_module.js

lib/internal/process/per_thread.js

lib/internal/process/pre_execution.js

lib/internal/modules/esm/fetch_module.js

codecov · 2024-10-16T18:22:40Z

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (fa8f149) to head (0bd51d5).
Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/fetch_module.js	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55407      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         652      653       +1     
  Lines      186918   187595     +677     
  Branches    36072    36113      +41     
==========================================
+ Hits       165270   165839     +569     
- Misses      14891    14975      +84     
- Partials     6757     6781      +24

Files with missing lines	Coverage Δ
lib/_http_agent.js	`98.17% <100.00%> (ø)`
lib/internal/modules/esm/resolve.js	`96.54% <100.00%> (+<0.01%)`	⬆️
lib/internal/modules/helpers.js	`98.80% <100.00%> (-0.20%)`	⬇️
lib/internal/process/per_thread.js	`99.30% <100.00%> (-0.01%)`	⬇️
lib/internal/process/pre_execution.js	`90.41% <100.00%> (-0.62%)`	⬇️
lib/internal/repl/utils.js	`95.72% <100.00%> (-0.02%)`	⬇️
lib/internal/url.js	`98.04% <100.00%> (+0.08%)`	⬆️
lib/internal/util/inspect.js	`99.95% <100.00%> (-0.05%)`	⬇️
lib/repl.js	`94.96% <100.00%> (ø)`
lib/internal/modules/esm/fetch_module.js	`42.85% <0.00%> (-0.53%)`	⬇️

... and 100 files with indirect coverage changes

gurgunday · 2024-10-17T10:45:16Z

@aduh95 can you take a look?

congrats on the release btw :)

lib/internal/util/inspect.js

mcollina

lgtm

nodejs-github-bot · 2024-10-18T12:45:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63178/

nodejs-github-bot · 2024-10-18T16:16:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63185/

gurgunday · 2024-10-19T09:58:22Z

@aduh95 will this merge? Or is there an issue

I got some other changes on the way but they will conflict with this one

nodejs-github-bot · 2024-10-19T10:06:02Z

Commit Queue failed

- Loading data for nodejs/node/pull/55407
✔  Done loading data for nodejs/node/pull/55407
----------------------------------- PR info ------------------------------------
Title      lib: remove startsWith/endsWith primordials for char checks (#55407)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     gurgunday:improve-startswith -> nodejs:main
Labels     lib / src, author ready, needs-ci, commit-queue-squash
Commits    5
 - lib: remove startsWith/endsWith primordials for char checks
 - lint: remove unused vars
 - revert: webidl2 change
 - use .length to check for empty string
 - add assert to `addNumericSeparator`
Committers 1
 - Gürgün Dayıoğlu <hey@gurgun.day>
PR-URL: https://github.com/nodejs/node/pull/55407
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55407
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - add assert to `addNumericSeparator`
   ℹ  This PR was created on Wed, 16 Oct 2024 16:53:41 GMT
   ✔  Approvals: 5
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/55407#pullrequestreview-2373132270
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/55407#pullrequestreview-2374915750
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55407#pullrequestreview-2375049155
   ✔  - Mattias Buelens (@MattiasBuelens): https://github.com/nodejs/node/pull/55407#pullrequestreview-2375222681
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/55407#pullrequestreview-2376217715
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-10-18T16:16:36Z: https://ci.nodejs.org/job/node-test-pull-request/63185/
- Querying data for job/node-test-pull-request/63185/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/11416653036

nodejs-github-bot · 2024-10-19T10:18:12Z

Landed in 8ed50bc

PR-URL: #55407 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mattias Buelens <mattias@buelens.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>

PR-URL: nodejs#55407 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mattias Buelens <mattias@buelens.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 16, 2024

lib: remove startsWith/endsWith primordials for char checks

3706c83

gurgunday force-pushed the improve-startswith branch from 3da1022 to 3706c83 Compare October 16, 2024 16:54

gurgunday changed the title ~~chore: remove startsWith/endsWith primordials for char checks~~ lib: remove startsWith/endsWith primordials for char checks Oct 16, 2024

JakobJingleheimer approved these changes Oct 16, 2024

View reviewed changes

lint: remove unused vars

1be2c43

revert: webidl2 change

e2ba06c

ljharb approved these changes Oct 16, 2024

View reviewed changes

lib/internal/modules/esm/fetch_module.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/resolve.js Show resolved Hide resolved

anonrig reviewed Oct 16, 2024

View reviewed changes

lib/internal/modules/esm/fetch_module.js Outdated Show resolved Hide resolved

lib/internal/process/per_thread.js Outdated Show resolved Hide resolved

lib/internal/process/pre_execution.js Show resolved Hide resolved

lib/internal/modules/esm/fetch_module.js Outdated Show resolved Hide resolved

use .length to check for empty string

5732d4d

gurgunday requested a review from anonrig October 16, 2024 20:55

aduh95 reviewed Oct 17, 2024

View reviewed changes

lib/internal/util/inspect.js Show resolved Hide resolved

aduh95 approved these changes Oct 17, 2024

View reviewed changes

mcollina approved these changes Oct 17, 2024

View reviewed changes

MattiasBuelens approved these changes Oct 17, 2024

View reviewed changes

mertcanaltin approved these changes Oct 17, 2024

View reviewed changes

Ethan-Arrowood approved these changes Oct 17, 2024

View reviewed changes

add assert to addNumericSeparator

0bd51d5

legendecas mentioned this pull request Oct 18, 2024

esm: optimize string checks and slicing with direct index access #55435

Closed

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2024

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 18, 2024

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 19, 2024

aduh95 approved these changes Oct 19, 2024

View reviewed changes

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 19, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2024

nodejs-github-bot merged commit 8ed50bc into nodejs:main Oct 19, 2024
75 checks passed

gurgunday deleted the improve-startswith branch October 19, 2024 10:21

aduh95 mentioned this pull request Oct 24, 2024

V23.1.0 proposal #55513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: remove startsWith/endsWith primordials for char checks #55407

lib: remove startsWith/endsWith primordials for char checks #55407

gurgunday commented Oct 16, 2024 •

edited

Loading

nodejs-github-bot commented Oct 16, 2024

panva commented Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

gurgunday commented Oct 17, 2024

mcollina left a comment

nodejs-github-bot commented Oct 18, 2024

nodejs-github-bot commented Oct 18, 2024

gurgunday commented Oct 19, 2024 •

edited

Loading

nodejs-github-bot commented Oct 19, 2024

nodejs-github-bot commented Oct 19, 2024

lib: remove startsWith/endsWith primordials for char checks #55407

lib: remove startsWith/endsWith primordials for char checks #55407

Conversation

gurgunday commented Oct 16, 2024 • edited Loading

nodejs-github-bot commented Oct 16, 2024

panva commented Oct 16, 2024

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

gurgunday commented Oct 17, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 18, 2024

nodejs-github-bot commented Oct 18, 2024

gurgunday commented Oct 19, 2024 • edited Loading

nodejs-github-bot commented Oct 19, 2024

nodejs-github-bot commented Oct 19, 2024

gurgunday commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

gurgunday commented Oct 19, 2024 •

edited

Loading