Skip to content

lib: fix ERR_INVALID_ARG_TYPE with --enable-source-maps#63215

Open
kimjune01 wants to merge 3 commits into
nodejs:mainfrom
kimjune01:fix/assert-source-maps-regression
Open

lib: fix ERR_INVALID_ARG_TYPE with --enable-source-maps#63215
kimjune01 wants to merge 3 commits into
nodejs:mainfrom
kimjune01:fix/assert-source-maps-regression

Conversation

@kimjune01

Copy link
Copy Markdown

When --enable-source-maps is enabled but no source map exists for the script (e.g. a plain .mjs file), getErrorSourceLocation() returned undefined. This caused getErrMessage() to return undefined, which made innerOk() pass [undefined] as the message tuple to innerFail(), hitting the ERR_INVALID_ARG_TYPE guard instead of throwing AssertionError.

Fix all code paths in getErrorSourceLocation() that returned bare undefined to fall back to the raw generated source line, matching the behavior when source maps are disabled entirely. Also add a guard for when V8 fails to provide a source line at all.

Includes tests for both .mjs and .cjs files under --enable-source-maps.

Fixes: #63169

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels May 9, 2026
@atlowChemi atlowChemi added the source maps Issues and PRs related to source map support. label May 10, 2026
@atlowChemi

Copy link
Copy Markdown
Member

@kimjune01 I think the commit's module should be lib, not assert, as I don't think this changes assert specifically, that is mostly the symptom, not the actual issue, no?

@kimjune01

Copy link
Copy Markdown
Author

Good point — the root cause is in the source map resolution path, not assert itself. I'll update the title.

@kimjune01 kimjune01 changed the title assert: fix ERR_INVALID_ARG_TYPE with --enable-source-maps lib: fix ERR_INVALID_ARG_TYPE with --enable-source-maps May 10, 2026
@idabeck-cpu

This comment was marked as spam.

@idabeck-cpu

This comment was marked as spam.

@idabeck-cpu

This comment was marked as spam.

@atlowChemi

Copy link
Copy Markdown
Member

Good point — the root cause is in the source map resolution path, not assert itself. I'll update the title.

@kimjune01 please reword the commit as well 🙏🏽
Let me know if any help is needed

@kimjune01
kimjune01 force-pushed the fix/assert-source-maps-regression branch from 629761d to bef74dd Compare May 11, 2026 04:00
@kimjune01

Copy link
Copy Markdown
Author

Done — reworded commit to lib: prefix.

@codecov

codecov Bot commented May 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (d4d35c6) to head (da520ed).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/errors/error_source.js 33.33% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #63215   +/-   ##
=======================================
  Coverage   90.12%   90.12%           
=======================================
  Files         743      743           
  Lines      242415   242422    +7     
  Branches    45656    45653    -3     
=======================================
+ Hits       218476   218491   +15     
  Misses      15438    15438           
+ Partials     8501     8493    -8     
Files with missing lines Coverage Δ
lib/internal/errors/error_source.js 82.55% <33.33%> (+0.73%) ⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@atlowChemi

Copy link
Copy Markdown
Member

@kimjune01 the CI failures all seem related to the changes

Comment thread test/parallel/test-assert-ok-source-maps.js Outdated
@kimjune01
kimjune01 force-pushed the fix/assert-source-maps-regression branch from 6eddc82 to 4b39cd1 Compare May 26, 2026 23:46
@kimjune01

Copy link
Copy Markdown
Author

Rebased onto current main and fixed the two lint failures:

  • Dropped the non-ASCII em-dash from the comments (lint-js-and-md)
  • Added the DCO Signed-off-by (lint-commit-message)

The earlier build failures looked unrelated to this lib/ change; should be clean on the fresh run.

@kimjune01
kimjune01 force-pushed the fix/assert-source-maps-regression branch from 4b39cd1 to 6a0b05f Compare May 26, 2026 23:49
Comment thread test/parallel/test-assert-ok-source-maps.js Outdated
@kimjune01

Copy link
Copy Markdown
Author

Done. Moved both cases into test-node-output-sourcemaps.mjs as snapshot fixtures under source-map/output/ with a // Flags: --enable-source-maps directive; dropped the standalone test.

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@legendecas legendecas added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 15, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 15, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Signed-off-by: June Kim <kimjune01@gmail.com>
Fold the .mjs and .cjs --enable-source-maps cases from the standalone
test-assert-ok-source-maps.js into the existing snapshot output test,
following the setup in test-node-output-sourcemaps.mjs as requested in
review. The fixtures live under test/fixtures/source-map/output/ with a
`// Flags: --enable-source-maps` directive and assert AssertionError
output via snapshots instead of bespoke spawnSync assertions.

Refs: nodejs#63169
Signed-off-by: June Kim <kimjune01@gmail.com>
The literal first argument to assert() is intentional in these
regression fixtures (the .snapshot files pin the exact source line and
column), so disable the rule inline rather than rewriting the call.
@kimjune01
kimjune01 force-pushed the fix/assert-source-maps-regression branch from b15e1c6 to da520ed Compare July 26, 2026 02:53
@kimjune01

Copy link
Copy Markdown
Author

Rebased onto main. Could someone run CI?

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. source maps Issues and PRs related to source map support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

assert: ERR_INVALID_ARG_TYPE on failing assert(value) under --enable-source-maps (v26 regression)

7 participants