fixed abort pending request of internetReachability

[gizomo]

AbortController.abort() throws DOMException with name AbortError. This exception rejects promise with reason declared in abort function attribute as I described at #699

[mikehardy]

Interesting, I have not personally played around with AbortController but this appears sane+correct based on my read of the relevant docs you helpfully linked. Thanks!

[mikehardy]

Some problematic test results:

src/internal/internetReachability.ts:82:62 - error TS2322: Type 'Timeout' is not assignable to type 'void'.

82     const timeoutPromise = new Promise<Response>((): void => timeoutHandle = setTimeout(
                                                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~
83       (): void => controller.abort('timedout'),
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
84       this._configuration.reachabilityRequestTimeout,
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
85     ));
   ~~~~~

src/internal/internetReachability.ts:90:61 - error TS2[32](https://github.com/react-native-netinfo/react-native-netinfo/actions/runs/7016281073/job/19104478038?pr=700#step:4:33)2: Type '() => void' is not assignable to type 'void'.

90     const cancelPromise = new Promise<Response>((): void => cancel = (): void => controller.abort('canceled'));
                                                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  src/internal/internetReachability.ts:90:61
    90     const cancelPromise = new Promise<Response>((): void => cancel = (): void => controller.abort('canceled'));
                                                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Did you mean to call this expression?

[gizomo]

Sorry. I hurried. I edited the file in Github. Didn't take into account return types.

[mikehardy]

CI behaving strangely - only closing so I can reopen which typically kicks off a CI round

[mikehardy]

Ok - will merge and release this, it seems reasonable, please please 🙏 keep an ear out in case there are issues here, hopefully this just works for others using this functionality

[matt-oakes]

🎉 This PR is included in version 11.1.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[Tymofiev]

@gizomo did this PR fix some real issue for you?

I am asking because it introduced one on our side because of a broken check by error message here. Since the explicit reject has been removed - abort message is 'Aborted' instead of 'timedout' | 'canceled' like expected.

cc @matt-oakes.

[mikehardy]

@Tymofiev - Matt Oakes no longer works on the repo so cc'ing him has no effect, it's just his user account key that still does releases

Anyway, I'm listening as I was afraid this may affect others.

Is there some reason you cannot inspect on the abort reason in order to differentiate, as mentioned in the issue related to the PR?

The right way is to use abort('canceled') to set reject reason to 'canceled' instead of DOMException.

[Tymofiev]

@mikehardy I am sorry, I meant to tag you here.

It is possible to inspect on the abort reason from the signal [1], but I was wondering if we could just revert the promise reject, so no extra work required to check signal reason.

Or we could use AbortSignal.timeout [2] instead of aborting with custom string reason and then check for error messages of abort.

[1]
...
    if (signal.aborted) {
      reject(signal.reason);
    }

    // Perform the main purpose of the API
    // Call resolve(result) when done.

    // Watch for 'abort' signals
    signal.addEventListener("abort", () => {
      // Stop the main operation
      // Reject the promise with the abort reason.
      reject(signal.reason);
    });
...

[2]
...
  if (e.name === "AbortError") {
    // Notify the user of abort.
  } else if (e.name === "TimeoutError") {
    // Notify the user of timeout
...

PS: Examples taken from https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal.

[mikehardy]

Reasonab PRs accepted but please coordinate philosophically with @gizomo

[Tymofiev]

@mikehardy yeah, okay, that's why I asked him the reason behind this change.

[gizomo]

if we could just revert the promise reject, so no extra work required to check signal reason.

Using an AbortController actually cancels the fetch. Rejecting a promise is not able to cancel the fetch, that can lead to a memory leak.

I don't advocate using the 'canceled' string as a reason. We can use DOMException checking e.name === "AbortError" or more safe to check controller.signal.aborted === true

Actually, we should revert reject('canceled') after controller.abort('canceled') as an option for browsers that do not fully support AbortController API.

[Tymofiev]

@gizomo thanks for the quick reply. I think it would still cancel the promise if you call abort without any reason, and reject is just to make sure we can track the exact error message.

Let's revert the reject to return the previous behaviour and as you said make sure it works in case AbortController API is not supported. Does it sound good?

PS: I am all into opening new PR to do that.