util: improve isInsideNodeModules by Uzlopak · Pull Request #52147 · nodejs/node

Uzlopak · 2024-03-18T20:58:36Z

Improve isInsideNodeModules by avoiding a Regex
kNodeModulesRE is using a non-capturing group

lib/internal/util.js

aduh95 · 2024-03-20T22:10:59Z

kNodeModulesRE is using a non-capturing group

Just curious, did you do that for performance reasons, or is that a neutral change to make the regex more intention-revealing?

nodejs-github-bot · 2024-03-20T22:17:59Z

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

Uzlopak · 2024-03-20T22:38:47Z

I just saw that the capturing group is not used and the captured string can be very long. So i had performance in mind, but i didnt wanted to claim a (significant?) performance gain, which i probably cant prove.

aduh95 · 2024-03-20T22:40:27Z

Looks like this a breaking the following tests:

parallel.test-buffer-constructor-outside-node-modules
parallel.test-buffer-constructor-node-modules-paths

Uzlopak · 2024-03-20T22:43:54Z

I will have a look at it in few hours ;)

Uzlopak · 2024-03-21T01:08:45Z

@aduh95

Should be fixed now.

nodejs-github-bot · 2024-03-21T08:38:30Z

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

nodejs-github-bot · 2024-03-21T22:30:44Z

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

nodejs-github-bot · 2024-03-22T11:06:33Z

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

Uzlopak · 2024-04-15T01:27:33Z

@nodejs/node-core-utils

can this be merged?

anonrig · 2024-04-15T02:01:02Z

lib/internal/util.js

-      if (RegExpPrototypeExec(/^\/|\\/, filename) === null)
+      if (
+        filename[0] !== '/' &&
+        StringPrototypeIncludes(filename, '\\') === false


This only applies to Windows right?

this PR is already old but afaik it was effecting linux too

nodejs-github-bot · 2024-05-04T11:57:22Z

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

nodejs-github-bot · 2024-05-05T08:45:43Z

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

nodejs-github-bot · 2024-05-05T09:45:26Z

Commit Queue failed

- Loading data for nodejs/node/pull/52147
✔  Done loading data for nodejs/node/pull/52147
----------------------------------- PR info ------------------------------------
Title      util: improve isInsideNodeModules (#52147)
Author     Aras Abbasi  (@Uzlopak)
Branch     Uzlopak:util-isInsideNodeModules -> nodejs:main
Labels     util, author ready, needs-ci
Commits    1
 - util: improve isInsideNodeModules
Committers 1
 - uzlopak 
PR-URL: https://github.com/nodejs/node/pull/52147
Reviewed-By: Antoine du Hamel 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52147
Reviewed-By: Antoine du Hamel 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 18 Mar 2024 20:58:36 GMT
   ✔  Approvals: 3
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/52147#pullrequestreview-1950310069
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/52147#pullrequestreview-1951180547
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/52147#pullrequestreview-1953430893
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-05-05T08:45:43Z: https://ci.nodejs.org/job/node-test-pull-request/58945/
- Querying data for job/node-test-pull-request/58945/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

PR-URL: nodejs#52147 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #52147 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs#52147 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

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 Mar 18, 2024

aduh95 reviewed Mar 18, 2024

View reviewed changes

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

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

anonrig reviewed Mar 19, 2024

View reviewed changes

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

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

Uzlopak force-pushed the util-isInsideNodeModules branch 2 times, most recently from d08b72a to 0778337 Compare March 20, 2024 22:00

aduh95 approved these changes Mar 20, 2024

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 20, 2024

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

Uzlopak closed this Mar 21, 2024

Uzlopak force-pushed the util-isInsideNodeModules branch from 0778337 to c714cda Compare March 21, 2024 01:07

Uzlopak reopened this Mar 21, 2024

BridgeAR approved these changes Mar 21, 2024

View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2024

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

lpinca approved these changes Mar 21, 2024

View reviewed changes

anonrig reviewed Apr 15, 2024

View reviewed changes

BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2024

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

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 5, 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 May 5, 2024

util: improve isInsideNodeModules

b77f32b

PR-URL: nodejs#52147 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

aduh95 force-pushed the util-isInsideNodeModules branch from f02a1ba to b77f32b Compare May 5, 2024 09:56

aduh95 merged commit b77f32b into nodejs:main May 5, 2024

Uzlopak deleted the util-isInsideNodeModules branch May 5, 2024 10:13

targos pushed a commit that referenced this pull request May 8, 2024

util: improve isInsideNodeModules

a42b93b

PR-URL: #52147 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

pimterry mentioned this pull request May 20, 2024

WASM crashes on Node v22.2.0 if it reaches the new Buffer() deprecation warning #53075

Closed

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

richardlau added the dont-land-on-v18.x label Aug 11, 2024

kravets-levko mentioned this pull request Aug 14, 2024

TypeError: Cannot read properties of undefined (reading '0') using client.openSession(); databricks/databricks-sql-nodejs#264

Open

Uh oh!

Conversation

Uzlopak commented Mar 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Mar 20, 2024

Uh oh!

nodejs-github-bot commented Mar 20, 2024

Uh oh!

Uzlopak commented Mar 20, 2024

Uh oh!

aduh95 commented Mar 20, 2024

Uh oh!

Uzlopak commented Mar 20, 2024

Uh oh!

Uzlopak commented Mar 21, 2024

Uh oh!

nodejs-github-bot commented Mar 21, 2024

Uh oh!

nodejs-github-bot commented Mar 21, 2024

Uh oh!

nodejs-github-bot commented Mar 22, 2024

Uh oh!

Uzlopak commented Apr 15, 2024

Uh oh!

anonrig Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uzlopak Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 4, 2024

Uh oh!

nodejs-github-bot commented May 5, 2024

Uh oh!

nodejs-github-bot commented May 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants