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

[v22.x backport] src: backport fast API changes by Renegade334 · Pull Request #59065 · nodejs/node · GitHub

/ node Public

[v22.x backport] src: backport fast API changes - #59065

Merged
aduh95 merged 3 commits into
nodejs:v22.x-stagingfrom
Renegade334:v22.x-backport-fast-api-changes
Jul 28, 2025
Merged

[v22.x backport] src: backport fast API changes#59065
aduh95 merged 3 commits into
nodejs:v22.x-stagingfrom
Renegade334:v22.x-backport-fast-api-changes

Conversation

Copy link
Copy Markdown
Member

Manual backport for #58896, which doesn't land cleanly on v22.x, and is needed to ensure that future PRs that touch the fast API will backport cleanly where possible.

Requires #58054 and #58489, both of which also need manual backporting.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/loaders
  • @nodejs/security-wg
  • @nodejs/url

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. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Jul 14, 2025
Renegade334 force-pushed the v22.x-backport-fast-api-changes branch from b42761a to 48174f1 Compare July 16, 2025 22:48
aduh95 requested review from anonrig and joyeecheung July 21, 2025 14:10

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

anonrig and others added 3 commits July 28, 2025 08:58
PR-URL: nodejs#58054
Backport-PR-URL: nodejs#59065
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
There are several motivation for removing this:

1. The implementation does not align with InternalModuleStat,
   most noticably it does not namespace the path or convert
   it to UTF-16 before using it with std::filesystem::path
   on Windows which could crash on non-English locale.
2. It needs the Environment - if not for decoding the string,
   at least for env->exec_path() to resolve the path for
   namespacing - and therefore needs a handle to the Context
   which requires a handle scope which actually makes the
   fast API version slower than the normal binding.

For simplicity this just removes the fast API to fix the bug and
improve the performance.

PR-URL: nodejs#58489
Backport-PR-URL: nodejs#59065
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#58896
Backport-PR-URL: nodejs#59065
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 force-pushed the v22.x-backport-fast-api-changes branch from 48174f1 to 6905258 Compare July 28, 2025 06:59

aduh95 commented Jul 28, 2025

Copy link
Copy Markdown
Contributor

Landed in 2fc8989...6905258

aduh95 merged commit 6905258 into nodejs:v22.x-staging Jul 28, 2025
19 checks passed
Renegade334 deleted the v22.x-backport-fast-api-changes branch July 28, 2025 07:35
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL