Skip to content
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

src: implement constants and natives binding directly #48186

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

src: implement natives binding without special casing

This patch removes special case in the internal binding loader
for natives, and implements it using the builtins internal
binding. Internally we do not actually need the natives binding,
so implement it as a legacy wrapper instead.

src: implement constants binding directly

Instead of adding a special case for it in the internal binding
loader, just implement it as usual using a per-context property
initializer.

This patch removes special case in the internal binding loader
for natives, and implements it using the builtins internal
binding. Internally we do not actually need the natives binding,
so implement it as a legacy wrapper instead.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/startup

@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. labels May 26, 2023
function isNativeUrl(url) {
url = SideEffectFreeRegExpPrototypeSymbolReplace(/\.js$/, url, '');

return StringPrototypeStartsWith(url, 'node:internal/') ||
ArrayPrototypeIncludes(PUBLIC_BUILTINS, url) ||
url in NATIVES || url === 'bootstrap_node';
Copy link
Member Author

@joyeecheung joyeecheung May 26, 2023

Choose a reason for hiding this comment

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

The original condition was already redundant, builtinIds actually contains both public and internal IDs, so I just removed it (as well as bootstrap_node which no longer exists)

Instead of adding a special case for it in the internal binding
loader, just implement it as usual using a per-context property
initializer.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Jun 9, 2023
This patch removes special case in the internal binding loader
for natives, and implements it using the builtins internal
binding. Internally we do not actually need the natives binding,
so implement it as a legacy wrapper instead.

PR-URL: #48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
joyeecheung added a commit that referenced this pull request Jun 9, 2023
Instead of adding a special case for it in the internal binding
loader, just implement it as usual using a per-context property
initializer.

PR-URL: #48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in 6a3403c...d102f18

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
This patch removes special case in the internal binding loader
for natives, and implements it using the builtins internal
binding. Internally we do not actually need the natives binding,
so implement it as a legacy wrapper instead.

PR-URL: #48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
Instead of adding a special case for it in the internal binding
loader, just implement it as usual using a per-context property
initializer.

PR-URL: #48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This patch removes special case in the internal binding loader
for natives, and implements it using the builtins internal
binding. Internally we do not actually need the natives binding,
so implement it as a legacy wrapper instead.

PR-URL: nodejs#48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Instead of adding a special case for it in the internal binding
loader, just implement it as usual using a per-context property
initializer.

PR-URL: nodejs#48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This patch removes special case in the internal binding loader
for natives, and implements it using the builtins internal
binding. Internally we do not actually need the natives binding,
so implement it as a legacy wrapper instead.

PR-URL: nodejs#48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Instead of adding a special case for it in the internal binding
loader, just implement it as usual using a per-context property
initializer.

PR-URL: nodejs#48186
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@ruyadorno
Copy link
Member

hi @joyeecheung these commits are not landing cleanly on v18.x-staging and will need manual backport in case we want to land this in v18.

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants