Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: v22.x-staging
Choose a base branch
from

Conversation

Renegade334
Copy link
Contributor

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.

anonrig and others added 3 commits July 14, 2025 07:48
PR-URL: nodejs#58054
Backport-PR-URL: nodejs#59065
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.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>
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>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot 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
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.

4 participants