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

Bad error message for assert.ok in ESM #50593

Closed
nicolo-ribaudo opened this issue Nov 7, 2023 · 7 comments
Closed

Bad error message for assert.ok in ESM #50593

nicolo-ribaudo opened this issue Nov 7, 2023 · 7 comments
Labels
assert Issues and PRs related to the assert subsystem. esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@nicolo-ribaudo
Copy link
Contributor

Version

21.1.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

const assert = require("assert");
 
assert.ok(0 === 2)
import assert from "assert"

assert.ok(0 === 2)

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

The CJS code throws this error:

➜ node file.js 
node:assert:399
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(0 === 2)

    at Object.<anonymous> (/Users/nic/Documents/misc/node-assert-test/file.js:3:8)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Module._load (node:internal/modules/cjs/loader:1023:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

I would expect something similar for ESM

What do you see instead?

ESM throws this error:

➜ node file.mjs
node:internal/process/esm_loader:40
      internalBinding('errors').triggerUncaughtException(
                                ^

AssertionError [ERR_ASSERTION]: false == true
    at file:///Users/nic/Documents/misc/node-assert-test/file.mjs:3:8
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Without showing the assert.ok call.

Additional information

No response

@tniessen tniessen added assert Issues and PRs related to the assert subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Nov 7, 2023
@txn100
Copy link

txn100 commented Nov 8, 2023

I think the variation you're seeing in error outputs between CJS and ESM is due to their different execution contexts—CJS is synchronous, while ESM is inherently asynchronous.

@MrJithil
Copy link
Member

MrJithil commented Nov 8, 2023

error code is same, so the message has to be.

Checking the implementation.

@MrJithil
Copy link
Member

MrJithil commented Nov 8, 2023

Both cases, the error iso originating from the assert.
In CJS, the error will reach directly to the file. But, for ESM, the esm_loader will catch and re-throw this exception using triggerUncaughtException internal bindings.

Also, inside the asserts, we are generating the message using getErrorMessage function. There we are prefixing the message The expression evaluated to a falsy value: for errors.

an attempt to open the file fd = openSync(filename, 'r', 0o666); is failing for ESM. So, the further message formatting being skipped.

static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {

@MrJithil
Copy link
Member

MrJithil commented Nov 8, 2023

#49913

@MrJithil
Copy link
Member

MrJithil commented Nov 9, 2023

For ESM, call.getFileName() is returning fileName as file://<path to file> instead of /<path to file>

const filename = call.getFileName();

MrJithil added a commit to MrJithil/node that referenced this issue Nov 9, 2023
This PR addresses issue nodejs#50593, which was caused by the design in the ESM loader. The ESM loader was modifying the file path and replacing the 'file' property with the file proto in the stack trace. This, in turn, led to unhandled exceptions when the assert module attempted to open the file to display erroneous code. The changes in this PR resolve this issue by handling the file path correctly, ensuring that the remaining message formatting code can execute as expected.
MrJithil added a commit to MrJithil/node that referenced this issue Nov 9, 2023
This PR addresses issue nodejs#50593, which was caused by the design in
the ESM loader.
The ESM loader was modifying the file path and replacing the 'file'
property with the file proto in the stack trace.
This, in turn, led to unhandled exceptions when the assert module
attempted to open the file to display erroneous code.
The changes in this PR resolve this issue by handling the file path
correctly, ensuring that the remaining message formatting code can
execute as expected.
@MrJithil
Copy link
Member

MrJithil commented Nov 9, 2023

Fix raised

@MrJithil
Copy link
Member

PR landed hence closing the issue. Thanks @nicolo-ribaudo for bringing this issue to notice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

No branches or pull requests

4 participants