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: use more defensive code when handling SWC errors #56646

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 17, 2025

In case the thrown error is not of the shape we expect, we get a TypeError but an ERR_INTERNAL_ASSERTION

@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 Jan 17, 2025
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.20%. Comparing base (2e45656) to head (7c1116a).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/typescript.js 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56646      +/-   ##
==========================================
- Coverage   89.21%   89.20%   -0.02%     
==========================================
  Files         662      662              
  Lines      191883   191886       +3     
  Branches    36941    36932       -9     
==========================================
- Hits       171196   171176      -20     
- Misses      13536    13547      +11     
- Partials     7151     7163      +12     
Files with missing lines Coverage Δ
lib/internal/modules/typescript.js 97.05% <50.00%> (-0.55%) ⬇️

... and 36 files with indirect coverage changes

@marco-ippolito
Copy link
Member

Not sure whats the benefit. That path should be unreachable. If it goes into the default it means its a bug or misuse of internals so user should report it.

@marco-ippolito marco-ippolito added the strip-types Issues or PRs related to strip-types support label Jan 19, 2025
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 22, 2025

Not sure whats the benefit. That path should be unreachable. If it goes into the default it means its a bug or misuse of internals so user should report it.

Yes exactly, hence the added assertion: if the thrown object is not of the shape we expect, we should throw an assertion fail (the current code assumes it's going to be of a certain shape)

@marco-ippolito
Copy link
Member

marco-ippolito commented Jan 22, 2025

ERR_INTERNAL_ASSERTION adds the

This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at 

Which is useful imho because it tells the user to open an issue and allows us to find bugs, while the normal assert is not so descriptive.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 22, 2025

I know, this PR doesn't change that

function fail(message) {
const ERR_INTERNAL_ASSERTION = lazyError();
throw new ERR_INTERNAL_ASSERTION(message);
}
assert.fail = fail;

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

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 23, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 23, 2025
@nodejs-github-bot nodejs-github-bot merged commit 905e4b4 into nodejs:main Jan 23, 2025
78 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 905e4b4

@aduh95 aduh95 deleted the swc-error-handling branch January 23, 2025 15:18
aduh95 added a commit that referenced this pull request Jan 27, 2025
PR-URL: #56646
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit that referenced this pull request Jan 30, 2025
PR-URL: #56646
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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. 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.

4 participants