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

Fix abstract coder filter and map types #4596

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KillariDev
Copy link

when running ethers with following libs & versions:
"better-typescript-lib": "2.6.0"
"typescript": "5.3.3"

I get following errors:

node_modules/ethers/lib.esm/abi/coders/abstract-coder.d.ts:41:5 - error TS2416: Property 'filter' in type 'Result' is not assignable to the same property in base type 'any[]'.
  Type '(callback: (el: any, index: number, array: Result) => boolean, thisArg?: any) => Result' is not assignable to type '{ <S extends any, This = undefined>(predicate: (this: This, value: any, index: number, array: this) => value is S, thisArg?: This | undefined): S[]; <This = undefined>(predicate: (this: This, value: any, index: number, array: this) => boolean, thisArg?: This | undefined): any[]; }'.
    Types of parameters 'callback' and 'predicate' are incompatible.
      Type '(this: any, value: any, index: number, array: this) => value is any' is not assignable to type '(el: any, index: number, array: Result) => boolean'.
        Types of parameters 'array' and 'array' are incompatible.
          Type 'Result' is not assignable to type 'this'.
            'Result' is assignable to the constraint of type 'this', but 'this' could be instantiated with a different subtype of constraint 'Result'.

41     filter(callback: (el: any, index: number, array: Result) => boolean, thisArg?: any): Result;
       ~~~~~~

node_modules/ethers/lib.esm/abi/coders/abstract-coder.d.ts:45:5 - error TS2416: Property 'map' in type 'Result' is not assignable to the same property in base type 'any[]'.
  Type '<T extends unknown = any>(callback: (el: any, index: number, array: Result) => T, thisArg?: any) => T[]' is not assignable to type '<U, This = undefined>(callbackfn: (this: This, value: any, index: number, array: this) => U, thisArg?: This | undefined) => Cast<{ [K in keyof this]: U; }, U[]>'.
    Types of parameters 'callback' and 'callbackfn' are incompatible.
      Types of parameters 'array' and 'array' are incompatible.
        Type 'Result' is not assignable to type 'this'.
          'Result' is assignable to the constraint of type 'this', but 'this' could be instantiated with a different subtype of constraint 'Result'.

45     map<T extends any = any>(callback: (el: any, index: number, array: Result) => T, thisArg?: any): Array<T>;
       ~~~

This pr fixes this typing issue

@ricmoo
Copy link
Member

ricmoo commented Feb 13, 2024

What is the root cause of the type issue? Were the node definitions changed in types?

I worry about making these sorts of changes as Ethers is widely used and so backwards compatibility is a hard requirement and changes that could break (even obscure) environments cannot be made…

@KillariDev
Copy link
Author

What is the root cause of the type issue? Were the node definitions changed in types?

I worry about making these sorts of changes as Ethers is widely used and so backwards compatibility is a hard requirement and changes that could break (even obscure) environments cannot be made…

This makes the map and filter types more strict and more accurate. Its true that it can break backward compatibility, maybe worth fixing for never version.

However, seems like this is a bigger can of worms than I initially thought. The type of array.map is complex. See this discussion: uhyo/better-typescript-lib#35 . And it seems like there's recommendation that array overrides should not be used as they can be security vulnerabilities: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/@@species

So I think in newer ethers versions, this array overloading should be removed all together?

@ricmoo
Copy link
Member

ricmoo commented Feb 28, 2024

Yes, in the v7 branch (local only) I don't override any of the Array method. I've got a whole different (less-intrusive) way to expose the Array as a keyed value as well, using a combination of WeakMaps and the Proxy. :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants