Skip to content

bootstrap: store internal loaders in C++ via a binding - #47215

Closed
joyeecheung wants to merge 2 commits into
nodejs:mainfrom
joyeecheung:loaders-return
Closed

bootstrap: store internal loaders in C++ via a binding#47215
joyeecheung wants to merge 2 commits into
nodejs:mainfrom
joyeecheung:loaders-return

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Mar 22, 2023

Copy link
Copy Markdown
Member

Instead of returning the internal loaders from the bootstrap script, we can simply call a binding to store them in C++. This eliminates the need for specializing the handling of this script.

@joyeecheung
bootstrap: store internal loaders in C++ via a binding
cc86b48
Instead of returning the internal loaders from the bootstrap script,
we can simply call a binding to store them in C++. This eliminates
the need for specializing the handling of this script.
@nodejs-github-bot

nodejs-github-bot commented Mar 22, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

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

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 22, 2023
anonrig
anonrig reviewed Mar 22, 2023
View reviewed changes
Comment threadsrc/node_builtins.cc
legendecas
legendecas approved these changes Mar 22, 2023
View reviewed changes

@legendecaslegendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. This looks much simpler without special handling.

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 23, 2023

Copy link
Copy Markdown
Collaborator

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

@legendecaslegendecas mentioned this pull request Mar 24, 2023
Merged
@legendecas

legendecas commented Mar 24, 2023

Copy link
Copy Markdown
Member

Linter is complaining.

@nodejs-github-bot

nodejs-github-bot commented Mar 24, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Mar 27, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Mar 27, 2023

Copy link
Copy Markdown
Collaborator

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

@legendecas

legendecas commented Mar 27, 2023

Copy link
Copy Markdown
Member

@nodejs/startup could we have another review on this?

@legendecaslegendecas 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. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 27, 2023
@nodejs-github-botnodejs-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 Mar 29, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@legendecaslegendecas 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 Mar 29, 2023
@nodejs-github-botnodejs-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 Mar 29, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@legendecas

legendecas commented Mar 29, 2023

Copy link
Copy Markdown
Member

Landed in c81e114

legendecas pushed a commit that referenced this pull request Mar 29, 2023
@joyeecheung@legendecas
bootstrap: store internal loaders in C++ via a binding
c81e114
Instead of returning the internal loaders from the bootstrap script,
we can simply call a binding to store them in C++. This eliminates
the need for specializing the handling of this script.
PR-URL: #47215
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@legendecaslegendecas removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Mar 29, 2023
@github-actionsgithub-actionsBot mentioned this pull request Mar 30, 2023
Open
18 tasks
@RafaelGSSRafaelGSS mentioned this pull request Apr 3, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@joyeecheung@RafaelGSS
bootstrap: store internal loaders in C++ via a binding
56296a7
Instead of returning the internal loaders from the bootstrap script,
we can simply call a binding to store them in C++. This eliminates
the need for specializing the handling of this script.
PR-URL: #47215
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Apr 6, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
@joyeecheung@RafaelGSS
bootstrap: store internal loaders in C++ via a binding
7fc2c57
Instead of returning the internal loaders from the bootstrap script,
we can simply call a binding to store them in C++. This eliminates
the need for specializing the handling of this script.
PR-URL: #47215
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS

RafaelGSS commented Apr 7, 2023

Copy link
Copy Markdown
Member

Hi @joyeecheung! I had to drop #46809 and this PR isn't landing cleanly on v19.x-staging. Can you please create a manual backport? If it strictly depends on #46809 feel free to change the label to backport-blocked-v19.x.

targos pushed a commit that referenced this pull request Nov 10, 2023
@joyeecheung@targos
bootstrap: store internal loaders in C++ via a binding
d668738
Instead of returning the internal loaders from the bootstrap script,
we can simply call a binding to store them in C++. This eliminates
the need for specializing the handling of this script.
PR-URL: #47215
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targostargos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
bootstrap: store internal loaders in C++ via a binding
4b01597
Instead of returning the internal loaders from the bootstrap script,
we can simply call a binding to store them in C++. This eliminates
the need for specializing the handling of this script.
PR-URL: nodejs/node#47215
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
bootstrap: store internal loaders in C++ via a binding
0e07176
Instead of returning the internal loaders from the bootstrap script,
we can simply call a binding to store them in C++. This eliminates
the need for specializing the handling of this script.
PR-URL: nodejs/node#47215
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonriganonriganonrig left review comments
@legendecaslegendecaslegendecas approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung@nodejs-github-bot@legendecas@RafaelGSS@anonrig@danielleadams