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

Typings don't compile #59

Closed
mrwillis opened this issue Sep 5, 2019 · 9 comments · Fixed by #74
Closed

Typings don't compile #59

mrwillis opened this issue Sep 5, 2019 · 9 comments · Fixed by #74

Comments

@mrwillis
Copy link

mrwillis commented Sep 5, 2019

There are two issues

node_modules/eth-sig-util/index.d.ts:16:5 - error TS2411: Property 'EIP712Domain' of type 'ITypedField[] | undefined' is not assignable to string index type 'ITypedField[]'.

16     EIP712Domain?: Array<ITypedField>
       ~~~~~~~~~~~~

I don't recall EIP712Domain being optional in the Types in the spec.

Also

export interface ITypedValue {
  [key: string]: string | number | ITypedValue | ITypedValue[]
}

should also allow for string[] and number[]

@alcuadrado
Copy link

I'm experiencing the same issue. I believe it's impossible to distribute a TS package that has this one as a dependency.

@mrwillis
Copy link
Author

mrwillis commented Oct 23, 2019

Yes it is impossible.

@danfinlay is it possible to get a fix for this? I notice in general the typings do not include anything under TypedDataUtils namespace even though it is indeed exposed.

@danfinlay
Copy link
Contributor

Sorry this has not been a high priority for me at the moment, I'll get to it when I can, or someone else can submit a PR for review.

@chentschel
Copy link
Contributor

@danfinlay Its not possible to use the npm with typescript. Can u please review the PR?

danfinlay added a commit that referenced this issue Nov 21, 2019
Make EIP712Domain non optional. Fix: #59
@danfinlay
Copy link
Contributor

I merged the open PR, fixed one of the two reported issues. Published as [email protected]

@danfinlay danfinlay reopened this Nov 21, 2019
@chentschel
Copy link
Contributor

@danfinlay thanks for the merge. As for the 2nd issue @mrwillis , wouldn't it be solved using a ITypedValue[] , defining ITypedValue[] as array of string, or number ?

export interface ITypedValue {
  [key: string]: string | number | ITypedValue | ITypedValue[]
}

@danfinlay
Copy link
Contributor

I'm willing to merge a PR either removing index.d.ts or improving it.

danfinlay added a commit that referenced this issue Nov 30, 2019
@danfinlay danfinlay mentioned this issue Nov 30, 2019
@danfinlay
Copy link
Contributor

Fine. Pending a proper typescript conversion, since I haven't had time to make sure this is all working right, and things clearly aren't, I've removed typedefs in [email protected].

I'm open to someone else doing the typescript conversion, and I may add a small gitcoin bounty to encourage it, and anyone else can too if the would value that feature.

@danfinlay
Copy link
Contributor

Opened issue for actually converting to Typescript: #70

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 a pull request may close this issue.

4 participants