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.

Sign up for GitHub

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

Jump to bottom

bootstrap: simplify initialization of source map handlers #48304

Closed
wants to merge 2 commits into from
Closed

bootstrap: simplify initialization of source map handlers #48304

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

  • Move the initialization of process.setSourceMapsEnabled and the maybeCacheGeneratedSourceMap callback to bootstrap/node.js so they are included in the snapshot.
  • Simplify the handling of --enable-source-maps by explicitly calling setSourceMapsEnabled() during pre-execution.

@joyeecheung
bootstrap: simplify initialization of source map handlers
d6d09ea 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @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. process Issues and PRs related to the process subsystem. labels Jun 2, 2023
VoltrexKeyva
VoltrexKeyva approved these changes Jun 2, 2023
View reviewed changes
lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved
jasnell
jasnell approved these changes Jun 3, 2023
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with the linting issues fixed.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 8, 2023
@nodejs-github-bot 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 Jun 8, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/48304
✔  Done loading data for nodejs/node/pull/48304
----------------------------------- PR info ------------------------------------
Title      bootstrap: simplify initialization of source map handlers (#48304)
Author     Joyee Cheung  (@joyeecheung)
Branch     joyeecheung:source-map-init -> nodejs:main
Labels     process, lib / src, needs-ci, commit-queue-squash
Commits    2
 - bootstrap: simplify initialization of source map handlers
 - fixup! bootstrap: simplify initialization of source map handlers
Committers 2
 - Joyee Cheung 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/48304
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: James M Snell 
Reviewed-By: Chengzhong Wu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48304
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: James M Snell 
Reviewed-By: Chengzhong Wu 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! bootstrap: simplify initialization of source map handlers
   ℹ  This PR was created on Fri, 02 Jun 2023 17:53:13 GMT
   ✔  Approvals: 4
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/48304#pullrequestreview-1458168465
   ✔  - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/48304#pullrequestreview-1458238733
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/48304#pullrequestreview-1459828667
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/48304#pullrequestreview-1464502401
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-06-08T08:43:09Z: https://ci.nodejs.org/job/node-test-pull-request/52158/
- Querying data for job/node-test-pull-request/52158/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/5216112521

joyeecheung added a commit that referenced this pull request Jun 9, 2023
@joyeecheung
bootstrap: simplify initialization of source map handlers
4ff55ec 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.

PR-URL: #48304
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in 4ff55ec

@joyeecheung joyeecheung closed this Jun 9, 2023
This was referenced Jun 11, 2023
Open
Open
Open
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@joyeecheung @RafaelGSS
bootstrap: simplify initialization of source map handlers
4066295 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.

PR-URL: #48304
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@joyeecheung @Ceres6
bootstrap: simplify initialization of source map handlers
b6c9295 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.

PR-URL: nodejs#48304
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@joyeecheung @Ceres6
bootstrap: simplify initialization of source map handlers
c860d01 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.

PR-URL: nodejs#48304
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@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
targos pushed a commit that referenced this pull request Nov 10, 2023
@joyeecheung @targos
bootstrap: simplify initialization of source map handlers
cf9ddcd 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.

PR-URL: #48304
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@targos targos 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: simplify initialization of source map handlers
3734b95 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.

PR-URL: nodejs/node#48304
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
bootstrap: simplify initialization of source map handlers
7cde3de 
- Move the initialization of process.setSourceMapsEnabled
  and the maybeCacheGeneratedSourceMap callback to
  bootstrap/node.js so they are included in the snapshot.
- Simplify the handling of --enable-source-maps by explicitly
  calling setSourceMapsEnabled() during pre-execution.

PR-URL: nodejs/node#48304
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@legendecas legendecas legendecas approved these changes

@anonrig anonrig anonrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joyeecheung @nodejs-github-bot @ruyadorno @jasnell @anonrig @legendecas @VoltrexKeyva