Preserve AbortController reason in CanceledError and restore CommonJS entrypoint#7518
Preserve AbortController reason in CanceledError and restore CommonJS entrypoint#7518sinteticx wants to merge 3 commits intoaxios:v1.xfrom
Conversation
CanceledError now stores the original abort reason (string or Error) and adapters/composeSignals pass signal.reason. Added unit tests. Closes axios#7434
Update field to point at (CommonJS build) instead of (ESM). Add to validate works in CommonJS environments without errors.
There was a problem hiding this comment.
4 issues found across 8 files
Confidence score: 3/5
- There is a concrete regression risk in
lib/helpers/composeSignals.js: abort listeners are added via wrapper callbacks but removed viaonabort, so cleanup likely fails and can leave stale listeners attached. - Most other findings are test reliability gaps rather than direct runtime breakage (
test/commonjs-compatibility.cjsnot running in CI, and swallowingaxios.getfailures), which reduces confidence that compatibility regressions will be caught. - Given one high-confidence, user-impacting logic issue (7/10) plus incomplete test coverage around abort behavior in
test/unit/abort-reason.js, this carries moderate merge risk and likely needs a fix or follow-up before relying on it. - Pay close attention to
lib/helpers/composeSignals.js,test/commonjs-compatibility.cjs, andtest/unit/abort-reason.js- listener cleanup appears incorrect and current tests may miss real compatibility/abort regressions.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/helpers/composeSignals.js">
<violation number="1" location="lib/helpers/composeSignals.js:52">
P1: Abort listeners are registered with wrapper callbacks but removed using `onabort`, so cleanup cannot detach them.</violation>
</file>
<file name="test/commonjs-compatibility.cjs">
<violation number="1" location="test/commonjs-compatibility.cjs:1">
P2: New CommonJS compatibility test file is not wired into any automated test command, so it will not run in CI.</violation>
<violation number="2" location="test/commonjs-compatibility.cjs:13">
P2: CommonJS compatibility script swallows `axios.get` failures, allowing success exit even when runtime request behavior is broken.</violation>
</file>
<file name="test/unit/abort-reason.js">
<violation number="1" location="test/unit/abort-reason.js:176">
P2: Tests are mislabeled as timeout/merged-signal coverage but only exercise manual `AbortController.abort(...)`, leaving real timeout/composed-signal logic untested.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| signals.forEach((signal) => signal.addEventListener('abort', onabort)); | ||
| signals.forEach((signal) => { | ||
| signal.addEventListener('abort', () => onabort(signal.reason)); |
There was a problem hiding this comment.
P1: Abort listeners are registered with wrapper callbacks but removed using onabort, so cleanup cannot detach them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/helpers/composeSignals.js, line 52:
<comment>Abort listeners are registered with wrapper callbacks but removed using `onabort`, so cleanup cannot detach them.</comment>
<file context>
@@ -43,7 +48,9 @@ const composeSignals = (signals, timeout) => {
- signals.forEach((signal) => signal.addEventListener('abort', onabort));
+ signals.forEach((signal) => {
+ signal.addEventListener('abort', () => onabort(signal.reason));
+ });
</file context>
| console.log('Status:', response.status); | ||
| }) | ||
| .catch(error => { | ||
| console.log('GET request failed, but axios loaded:', error.message); |
There was a problem hiding this comment.
P2: CommonJS compatibility script swallows axios.get failures, allowing success exit even when runtime request behavior is broken.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/commonjs-compatibility.cjs, line 13:
<comment>CommonJS compatibility script swallows `axios.get` failures, allowing success exit even when runtime request behavior is broken.</comment>
<file context>
@@ -0,0 +1,14 @@
+ console.log('Status:', response.status);
+ })
+ .catch(error => {
+ console.log('GET request failed, but axios loaded:', error.message);
+ });
\ No newline at end of file
</file context>
| @@ -0,0 +1,14 @@ | |||
| const axios = require('../'); | |||
There was a problem hiding this comment.
P2: New CommonJS compatibility test file is not wired into any automated test command, so it will not run in CI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/commonjs-compatibility.cjs, line 1:
<comment>New CommonJS compatibility test file is not wired into any automated test command, so it will not run in CI.</comment>
<file context>
@@ -0,0 +1,14 @@
+const axios = require('../');
+
+console.log('Axios loaded successfully in CommonJS environment');
</file context>
| @@ -0,0 +1,214 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
P2: Tests are mislabeled as timeout/merged-signal coverage but only exercise manual AbortController.abort(...), leaving real timeout/composed-signal logic untested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/unit/abort-reason.js, line 176:
<comment>Tests are mislabeled as timeout/merged-signal coverage but only exercise manual `AbortController.abort(...)`, leaving real timeout/composed-signal logic untested.</comment>
<file context>
@@ -0,0 +1,214 @@
+ }
+ });
+
+ it('should work with merged signals from two controllers', async () => {
+ const timeoutController = new AbortController();
+ const userController = new AbortController();
</file context>
|
Hi @davecardwell, Could you review my changes |
Summary
Fixes a regression introduced in 1.13.6 where require('axios') in a CommonJS (Node.js) project would throw:
Cannot use import statement outside a module.
This was caused by package.json’s main field pointing to index.js (ESM), even though the intended CommonJS entrypoint exists at axios.cjs
Changes
Why This Matters
This prevents downstream CommonJS-only projects (and nested deps) from breaking due to accidentally loading ESM source files via require().
Summary by cubic
Restores
axios’s CommonJS entrypoint sorequire('axios')works in Node CJS, and preserves AbortController abort reasons inCanceledErrorfor clearer cancellation errors across adapters and composed signals.Description
package.jsonmain to./dist/node/axios.cjs.signal.reasonthrough HTTP/XHR adapters andcomposeSignals.CanceledErrornow stores.reasonand uses it as the message when none is provided.require('axios')threw an ESM import error.AxiosError.ERR_CANCELED.Docs
CanceledErrorexposes.reason(string or Error). IfAbortController.abort('foo')is used, the error message becomes'foo'.require('axios')loads the CJS build.Testing
test/unit/abort-reason.jsto verify message and.reasonfor string/Error/no-reason cases and merged signals.test/commonjs-compatibility.cjsto confirmrequire('axios')works and a basic GET runs.Written for commit eea799a. Summary will update on new commits.