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

esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION #54868

Merged
merged 1 commit into from
Sep 11, 2024
Merged

esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION #54868

merged 1 commit into from
Sep 11, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 9, 2024

With --experimental-detect-module being on by default, trying to require() a file with ESM syntax would result in an failed assertion if --experimental-require-module was not passed. Instead of an assertion, let's throw the usual error.

Fixes: #54773

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Sep 9, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.89%. Comparing base (e1e312d) to head (d776567).
Report is 1151 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54868      +/-   ##
==========================================
- Coverage   87.90%   87.89%   -0.01%     
==========================================
  Files         651      651              
  Lines      183343   183355      +12     
  Branches    35710    35722      +12     
==========================================
- Hits       161165   161164       -1     
+ Misses      15466    15461       -5     
- Partials     6712     6730      +18
Files with missing lines Coverage Δ
lib/internal/modules/esm/loader.js 97.76% <100.00%> (+<0.01%) ⬆️

... and 27 files with indirect coverage changes

@aduh95 aduh95 requested a review from joyeecheung September 10, 2024 12:30
@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 Sep 10, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

Failed to start CI
- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 400 Bad Request
https://github.com/nodejs/node/actions/runs/10796198474

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Sep 10, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

Failed to start CI
- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 400 Bad Request
https://github.com/nodejs/node/actions/runs/10796366576

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Sep 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2024
@nodejs-github-bot nodejs-github-bot merged commit 53cdffe into nodejs:main Sep 11, 2024
72 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 53cdffe

@aduh95 aduh95 deleted the fix-assertion-error branch September 11, 2024 22:32
@mxschmitt
Copy link

mxschmitt commented Sep 12, 2024 •
edited
Loading

@aduh95 can we get this backported to v22?

aduh95 added a commit that referenced this pull request Sep 12, 2024
@aduh95
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
58b7ea2 
PR-URL: #54868
Fixes: #54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 added a commit that referenced this pull request Sep 13, 2024
@aduh95
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
297bd24 
PR-URL: #54868
Fixes: #54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 added a commit that referenced this pull request Sep 13, 2024
@aduh95
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
a624002 
PR-URL: #54868
Fixes: #54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
@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 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@aduh95 @louwers
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
64483e5 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@aduh95 @tpoisseau
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
afabd10 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 5, 2025
@aduh95 @joyeecheung
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
3f9a486 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 5, 2025
@aduh95 @joyeecheung
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
285f0fa 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 5, 2025
@aduh95 @joyeecheung
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
4d5f371 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 5, 2025
@aduh95 @joyeecheung
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
a5baa26 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@joyeecheung joyeecheung added backport-open-v20.x Indicate that the PR has an open backport and removed dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Feb 6, 2025
@joyeecheung
Copy link
Member

Backport in #56927

joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 6, 2025
@aduh95 @joyeecheung
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
8449085 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 6, 2025
@aduh95 @joyeecheung
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
7c94218 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Feb 7, 2025
Closed
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 10, 2025
@aduh95 @joyeecheung
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
11f03fa 
PR-URL: nodejs#54868
Fixes: nodejs#54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Feb 11, 2025
@aduh95 @marco-ippolito
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION
4813a6a 
PR-URL: #54868
Backport-PR-URL: #56927
Fixes: #54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Refs: #52697
@marco-ippolito marco-ippolito mentioned this pull request Mar 6, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avivkeller avivkeller avivkeller left review comments

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v20.x Indicate that the PR has an open backport esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
8 participants
@aduh95 @nodejs-github-bot @mxschmitt @joyeecheung @jasnell @lpinca @avivkeller @targos