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

Usage of AbortController ist breaking for setup without DOM Library #4124

Closed
smirowstanitzok opened this issue Oct 2, 2021 · 10 comments · Fixed by #4229
Closed

Usage of AbortController ist breaking for setup without DOM Library #4124

smirowstanitzok opened this issue Oct 2, 2021 · 10 comments · Fixed by #4229

Comments

@smirowstanitzok
Copy link

Describe the bug

Axios has been using the AbortController from the DOM library since version 0.22.0 (# 3305). Since then, the DOM library has to be stored in tsconfig.json. Without DOM the new version is breaking for our setup and can no longer be used.

To Reproduce

Use axios.get() without DOM Library. To reproduce change tsconfig.json to eg.:

...
    "lib": ["ES2020"],
...

Expected behavior

axios should work without DOM. Maybe there is an alternative for AbortController?

Environment

  • Axios Version 0.22.0
  • Adapter http
  • Browser none
  • Node.js Version 14.18.0
  • OS: macOS 11.6

Additional context/Screenshots

Given Exception:

> @smirowstanitzok/trendytag-sdk@3.1.2 build /home/runner/work/trendytag-sdk/trendytag-sdk
> tsc

Error: node_modules/axios/index.d.ts(81,12): error TS2304: Cannot find name 'AbortSignal'.
Error: src/index.ts(189,13): error TS2322: Type 'unknown' is not assignable to type 'AxiosResponse<ContactFormResponse> | undefined'.
  Type 'unknown' is not assignable to type 'AxiosResponse<ContactFormResponse>'.
Error: src/index.ts(189,88): error TS2345: Argument of type 'ContactForm' is not assignable to parameter of type 'ContactFormResponse'.
  Property 'messageId' is missing in type 'ContactForm' but required in type 'ContactFormResponse'.
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! @smirowstanitzok/trendytag-sdk@3.1.2 build: `tsc`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the @smirowstanitzok/trendytag-sdk@3.1.2 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2021-10-01T08_27_20_612Z-debug.log
Error: Process completed with exit code 2.
@DigitalBrainJS
Copy link
Collaborator

DigitalBrainJS commented Oct 2, 2021

I guess importing typings from node-abort-controller polyfill for nodejs environment will fix this. Btw node v15+ has native AbortController implementation.

@jan-molak
Copy link

In the past, whenever Axios has accidentally started to rely on DOM-only types, the preferred fix was to revert such types back to any, see #3228 and #3237. I'd be happy to prepare a PR if that's the preferred way to fix it in this case too?

Btw node v15+ has native AbortController implementation.

Good point. However, Node 15 is an "experimental" branch and Node 16 LTS still has a month left before becoming the "active" development line.

My suggestion would be to fix the current issue by setting the type to any, and then reverting it back to AbortController when Node 16 becomes the new "active" line.

jan-molak added a commit to serenity-js/serenity-js that referenced this issue Oct 4, 2021
@DigitalBrainJS
Copy link
Collaborator

I don't think it's really necessary to define a more generic type any | object for AbortController since:

  • node.js v15 is almost at the LTS stage, I guess there are a few months left
  • we will make the typing weaker for the client build too
  • it's easy to import the AbortController polyfill that provides TS types - the packet size of a few KB doesn't really matter for the server-side, even if you don't use AbortController in the server code yet.
    But this is just my personal opinion, and since I do not use TS, I am not too concerned about typings exactness.

@pbrennand-francis
Copy link

This just created a fairly gnarly build issue via another library (NestJS) which uses Axios, so I'd like to second the "setting the type to any" solution, if only to minimise the number of engineers who need to trace this down.

@jan-molak
Copy link

  • we will make the typing weaker for the client build too

You're right, that's the consequence of using any.

An alternative would be for Axios to provide its own copy of AbortController, AbortSignal, AbortSignalEventMap until Node 16 reaches LTS (source). This doesn't seem too terrible and provides stronger typing for both front-end and back-end implementations at the cost of slight overheard over a couple of months for Axios:

/** A controller object that allows you to abort one or more DOM requests as and when desired. */
interface AbortController {
    /**
     * Returns the AbortSignal object associated with this object.
     */
    readonly signal: AbortSignal;
    /**
     * Invoking this method will set this object's AbortSignal's aborted flag and signal to any observers that the associated activity is to be aborted.
     */
    abort(): void;
}

declare var AbortController: {
    prototype: AbortController;
    new(): AbortController;
};

interface AbortSignalEventMap {
    "abort": Event;
}

/** A signal object that allows you to communicate with a DOM request (such as a Fetch) and abort it if required via an AbortController object. */
interface AbortSignal extends EventTarget {
    /**
     * Returns true if this AbortSignal's AbortController has signaled to abort, and false otherwise.
     */
    readonly aborted: boolean;
    onabort: ((this: AbortSignal, ev: Event) => any) | null;
    addEventListener<K extends keyof AbortSignalEventMap>(type: K, listener: (this: AbortSignal, ev: AbortSignalEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void;
    addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;
    removeEventListener<K extends keyof AbortSignalEventMap>(type: K, listener: (this: AbortSignal, ev: AbortSignalEventMap[K]) => any, options?: boolean | EventListenerOptions): void;
    removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions): void;
}

declare var AbortSignal: {
    prototype: AbortSignal;
    new(): AbortSignal;
};

However, please note that when I proposed a similar solution for ProgressEvent back in #3228, @chinesedfan has chosen an alternative fix using any instead (#3227). So with the above proposal, I wanted to stay compatible with their design choices..

  • it's easy to import the AbortController polyfill that provides TS types - the packet size of a few KB doesn't really matter for the server-side, even if you don't use AbortController in the server code yet.

I believe Axios should work for both front-end and back-end without resorting to force developers to use polyfills for the current LTS versions of major runtime environments, such as Node.


@chinesedfan - apologies for tagging you in this reply, but could you please let me know which design direction you'd prefer (any vs a copy of AbortController) and I'd be happy to propose a PR.

Thanks,
Jan

@matushorvath
Copy link

Node.js 16 is now LTS. Could someone from the Axios project please comment whether this issue is going to be fixed to still support node.js 14, or whether the official position is that Axios now requires node.js 15 or newer? Personally I would prefer if Axios could support node.js 14 still, especially since it's such a simple fix, and not force everyone to update to 16 the moment it became LTS. Anyway, either decision will work, but it would be nice to have a statement here, so we at least know whether to wait for a fix or whether we should start adding polyfills to make axios work with node 14. Thank you (and appreciate your work on Axios). Matus

@armandodlvr
Copy link

Does anyone know if the PR is going to be merge soon? #4229

@andynunes
Copy link

+1 This is causing an issue when upgrading axios in a NuxtJS project as well, which is only needed to avoid a vulnerability in earlier versions.

This solution allows upgrading to the latest version of axios without forcing an upgrade to Node v16, which is important.

@nyalsadiq
Copy link

Any chance a fix will be merged soon? Is there anything we can do to help? :)

@alexcza
Copy link

alexcza commented May 4, 2022

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants