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

module: add sourceURL magic comment hinting generated source #54402

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

legendecas
Copy link
Member

Source map is not necessary in strip-only mode. However, to map the
source file in debuggers to the original TypeScript source, add a
sourceURL magic comment to hint that it is a generated source.

This has no runtime side-effects. --enable-source-maps will skip
sources without sourceMappingURL magic comments since error stack
remapping is not needed.

This improves debugger experiences like VSCode JS Debugger allowing
setting breakpoints on the original typescript source files, instead
of popping up the stripped source contents.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Aug 15, 2024
@legendecas legendecas added inspector Issues and PRs related to the V8 inspector protocol strip-types Issues or PRs related to strip-types support labels Aug 15, 2024
Source map is not necessary in strip-only mode. However, to map the
source file in debuggers to the original TypeScript source, add a
sourceURL magic comment to hint that it is a generated source.
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.31%. Comparing base (2c14615) to head (95bbe2e).
Report is 381 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54402      +/-   ##
==========================================
+ Coverage   87.08%   87.31%   +0.23%     
==========================================
  Files         648      648              
  Lines      182341   182362      +21     
  Branches    34982    34984       +2     
==========================================
+ Hits       158783   159224     +441     
+ Misses      16831    16392     -439     
- Partials     6727     6746      +19     
Files with missing lines Coverage Δ
lib/internal/modules/helpers.js 97.57% <100.00%> (+0.01%) ⬆️

... and 55 files with indirect coverage changes

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 16, 2024
@legendecas
Copy link
Member Author

Stress test on rhel8-arm64: https://ci.nodejs.org/job/node-stress-single-test/523

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2024
@legendecas
Copy link
Member Author

legendecas commented Aug 20, 2024

Improving the test case to be more stable on inspector disconnection. It should be a separate issue (with about 1/20 reproduce rate).

backtrace of SEGV
Thread 7 Crashed:
0   node                          	       0x100e99604 node::inspector::TcpHolder::WriteRaw(std::__1::vector> const&, void (*)(uv_write_s*, int)) + 48 (inspector_socket.cc:733)
1   node                          	       0x100e9b798 node::inspector::ProtocolHandler::WriteRaw(std::__1::vector> const&, void (*)(uv_write_s*, int)) + 20 (inspector_socket.cc:666) [inlined]
2   node                          	       0x100e9b798 node::inspector::(anonymous namespace)::WsHandler::Write(std::__1::vector>) + 276 (inspector_socket.cc:406)
3   node                          	       0x100e99d24 node::inspector::InspectorSocket::Write(char const*, unsigned long) + 104 (inspector_socket.cc:819)
4   node                          	       0x100e90830 node::inspector::(anonymous namespace)::RequestToServer::Dispatch(node::inspector::InspectorSocketServer*) const + 52 (inspector_io.cc:83) [inlined]
5   node                          	       0x100e90830 node::inspector::(anonymous namespace)::RequestQueueData::DoDispatch() + 232 (inspector_io.cc:153) [inlined]
6   node                          	       0x100e90830 node::inspector::(anonymous namespace)::RequestQueueData::RequestQueueData(uv_loop_s*)::'lambda'(uv_async_s*)::operator()(uv_async_s*) const + 244 (inspector_io.cc:105) [inlined]
7   node                          	       0x100e90830 node::inspector::(anonymous namespace)::RequestQueueData::RequestQueueData(uv_loop_s*)::'lambda'(uv_async_s*)::__invoke(uv_async_s*) + 272 (inspector_io.cc:102)
8   node                          	       0x101ac2c04 uv__async_io + 276 (async.c:176)
9   node                          	       0x101ad5ef4 uv__io_poll + 1032 (kqueue.c:385)
10  node                          	       0x101ac319c uv_run + 412 (core.c:448)
11  node                          	       0x100e8fe54 node::inspector::InspectorIo::ThreadMain() + 1632 (inspector_io.cc:319)
12  libsystem_pthread.dylib       	       0x18d571f94 _pthread_start + 136
13  libsystem_pthread.dylib       	       0x18d56cd34 thread_start + 8

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas 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 Aug 20, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 20, 2024
@nodejs-github-bot nodejs-github-bot merged commit 5376e69 into nodejs:main Aug 20, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 5376e69

@legendecas legendecas deleted the ts/source-url branch August 20, 2024 20:24
RafaelGSS pushed a commit that referenced this pull request Aug 25, 2024
Source map is not necessary in strip-only mode. However, to map the
source file in debuggers to the original TypeScript source, add a
sourceURL magic comment to hint that it is a generated source.

PR-URL: #54402
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 25, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. inspector Issues and PRs related to the V8 inspector protocol module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants