Add a possibility to cancel a request. Resolves #16

[dhuebner]

Manually tested.

TODO:

• Discuss the API
• Add tests

[spoenemann]

Nice! I'd consider to adapt the naming to other libraries to avoid confusion, see remarks below.

Please remove packages/vscode-messenger-devtools/webview-ui/package-lock.json – there should be only a single package-lock in the root.

[dankeboy36]

Hello 👋 @dhuebner, do you plan to address the review remarks regarding the API? Thank you!

[dhuebner]

@dankeboy36
Hi! I can have a look at end of this week, I hope.

[spoenemann]

@dhuebner we should consider using AbortSignal as suggested in #16. It would be nice to do things the standard way if possible.

[dhuebner]

@spoenemann

we should consider using AbortSignal as suggested in #16

Using an own Implementation makes us more flexible in implementing new things and extending existing. I think this is also a reason why vs-code has not re-used AbortSignal but implemented something own.

Other suggestion were applied! The only open conversation is #17 (comment) where I didn't get your concerns. Maybe we can chat in Around about that?

[spoenemann]

This is in good shape, thank you! A few more remarks before we can merge it.

packages/vscode-messenger-devtools/webview-ui/package-lock.json should be removed – there should be only one lock file in the project root.

Idea: is it possible to add an adapter class that creates a CancellationToken for a given AbortSignal? I think the concepts are very similar. Internally, it's a good idea to stay compatible with the way vscode-jsonrpc handles this.

[spoenemann]

I think appending catch to finally has different semantics than the other way around. See also corresponding code in the webview.

[dhuebner]

I think appending catch to finally has different semantics

I thought it is basically the same as:

pendingRequest.result.finally(() => {
    // Request finished, remove listener
    listener.dispose();
});
pendingRequest.result.catch((err) =>
    this.log(`Pending request rejected: ${String(err)}`)
);

Is it not just the execution order of attached callbacks that changes? Which is in this case doesn't matter.

[spoenemann]

I'm not sure. My impression after reading https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally#return_value is that the promise returned by finally behaves differently from the original promise.

[dhuebner]

It would be strange, if the kind of how you use the API (directly or chained) would change the behavior. But I will not wonder if it is like that in JS... However, I've tried different order and direct/chained here and it behaves the same.

[dhuebner]

packages/vscode-messenger-devtools/webview-ui/package-lock.json should be removed – there should be only one lock file in the project root.

This is because devtools package is not part of the main build. I already prepared a branch to add it to the main build. See: https://github.com/TypeFox/vscode-messenger/tree/dhuebner/add_DevTools_toBuild

I will open a PR after this one is merged.

Idea: is it possible to add an adapter class that creates a CancellationToken for a given AbortSignal? I think the concepts are very similar. Internally, it's a good idea to stay compatible with the way vscode-jsonrpc handles this.

Should it be a part of this PR, on can we do it later?

[spoenemann]

We can do it later, I was just curious of the new API proposed here is already conceptually compatible with AbortSignal, or we need to change something.

[spoenemann]

@dhuebner could you try this?

export function createCancellationToken(signal: AbortSignal): CancellationToken {
    return {
        get isCancellationRequested(): boolean {
            return signal.aborted;
        },

        onCancellationRequested: (callback: (reason: string) => void) => {
            const listener = () => callback(String(signal.reason));
            signal.addEventListener('abort', listener);
            return {
                dispose: () => signal.removeEventListener('abort', listener)
            }
        }
    };
}

[dhuebner]

@dhuebner could you try this?

Works, I also added an additional test.

[spoenemann]

Isn't AbortSignal available for Node.js, too? Then we could move this to the common package.

[dhuebner]

I think the node version has a slightly different method signatures. In my case AbortController.abort() has no reason parameter. I also not sure how common AbortSignal in vs-code extension context is.