Coc.nvim: Global patch to promise prototype causes typescript errors in other packages

[antonk52]

Result from CocInfo

Coc functionality is not effected, CocInfo output is irrelevant for this issue.

Describe the bug

coc.nvim patches promise prototype to add logError globally (file), this causes issues when developing coc plugins using typescript. In my example it is a thrid party code postcss which has a line of code such as

class LazyResult implements Promise<Result> {}

Reproduce the bug

I've created a repository to reproduce the issue.

When attempting to build using typescript you will see the following error

node_modules/postcss/lib/lazy-result.d.ts:16:22 - error TS2420: Class 'LazyResult' incorrectly implements interface 'Promise<Result>'.
  Property 'logError' is missing in type 'LazyResult' but required in type 'Promise<Result>'.

16 export default class LazyResult implements Promise<Result> {
                        ~~~~~~~~~~

  node_modules/coc.nvim/lib/util/extensions.d.ts:6:5
    6     logError(): void;
          ~~~~~~~~~~~~~~~~~
    'logError' is declared here.


Found 1 error.

A potential fix would be to avoid patching promise globally and to log errors where it is needed manually/adding a util promise wrapper for this purpose.

[chemzqm]

The prototype of Promise could be changed in standard js as well, make your tsc not check thet file.

[chemzqm]

It's not good design to add method to builtin object, but it worse to try to implement same interface for builtin object, it should use something like thenable of vscode.

[antonk52]

I would like to ask you to reconsider. Globally patching prototype is an anti pattern (MDN) in the js ecosystem. This may be acceptable in smaller projects that do not share a limited runtime. Coc.nvim is not a small project and it both has it's own dependencies and there are a bunch of plugins that depend on coc.nvim itself. You proposed solution of ignoring the file that throws the error is not optimal since it is no better than @ts-ignore or any. Vscode's Thenable is also not an option since unlike coc.nvim postcss can be run in a browser.

Coc.nvim is a platform that provides other people to build plugins to enhance and improve developer experience. I believe coc.nvim should make it easier to write new plugin and improve what we already have. Even though I may disagree with some of the design decisions in the postcss code base, their code is encapsulated in their npm package. While coc.nvim pollutes the entire runtime once imported/required.

Extracting the logger in a single function helps to maintain the encapsulation and keep a sane dependency graph of the code. It can also lower the gap for the newer contributors since the logic is easily traceable including github's built in go to definition functionality.

Alternately I can edit the code mod to instead of having it as a wrapper function to purely log errors in place

// old
aPromise.logError()

// proposed
import {logExtensionError} from 'util/extensions'
aPromise.catch(logExtensionError)

[Ceda-EI]

I agree with @antonk52 about polluting Promise prototype. It is generally accepted to be a bad practice and shouldn't be used. In this particular case, it doesn't even provide enough value to justify it's creation. It simply makes a line of code shorter by a few characters which imo is not worth opting for a bad practice.