-
Notifications
You must be signed in to change notification settings - Fork 122
Add TypeDeclarations #58
base: master
Are you sure you want to change the base?
Conversation
…enerated by contributors who may use an IDE
…(and add tsc to build)
The provider argument can be of type 'provider' from web3-core (e.g httpProvider). We may want to allow 'any' here
Apologies, I wiped this out whilst testing. We will need to bump the version number though. I have not included any .d.ts definition files in the project itself, but they are added to the dist folder on build. I can't think of any reason to add them to version control, even though some other projects do (aws-sdk)
We will probably need to go through and check carefully which parameters are optional for all exported classes/functions We should also double check that all the types are correct (or ask someone very familar with the library who would be happy to QA everything)
src/utils/records.js
Outdated
@@ -1,6 +1,20 @@ | |||
import { addressUtils } from '@0xproject/utils' | |||
import { validateContent } from './contents' | |||
|
|||
/** | |||
* @typedef { "address" | "content" | "oldcontent" } recordType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mixed up type and contentType (oldcontent| contenthash | null).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot. Should recordType
have just the following: * @typedef { "address" | "content" } recordType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think recordType
can be null as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be added now
src/index.js
Outdated
@@ -239,6 +334,11 @@ class Name { | |||
} | |||
} | |||
|
|||
/** | |||
* Get Resolver | |||
* @param {string} coinId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make coinId
of getAddress()
to be optional? It falls back to ETH
if it doesn't have an argument so something like https://github.com/ensdomains/ensjs/blob/master/src/__tests__/ens.test.js#L224 will fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual fallback logic is here https://github.com/ensdomains/ensjs/blob/7311aebd1125101d8ddaef28b5d3aa4bdc917b57/src/index.js#L244
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Question actually, do most smart contracts these days prevent sending to 0x000.. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question actually, do most smart contracts these days prevent sending to 0x000.. ?
that is not relevant to this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes you are right, it is the resolver address, not the smart contract address. I wasn't quite sure if resolvers were smart contracts too or not. Thanks for the clarification
There was a conversation about rebuilding the entire library in typescript at some point, @makoto would have a better idea whether this is going ahead or not. How involved would you be interested in getting @taylorjdawson if needed? |
@taylorjdawson apologies for not getting back sooner, is this something you fancy doing, or know anyone who could? |
What's the status on the TS conversion / rewrite @makoto @taylorjdawson ? @timhc22 If there is any help needed, I'd be happy to. |
@gomesalexandre @taylorjdawson, the integration which I wrote was just to add type declarations, however, @makoto was interested in rewriting the whole library in Typescript, so what I did can probably be discarded if it can be superseded by something better. Alternately, things might be working differently now that ens has switched over to a dao model. |
That's a good point on ENS being a DAO now. I'll get involved there to see what's the path forward on this! |
We should check all these work properly before merging.
I have tested by importing in my own project using this npm package (from which I am doing the pull request):
@unegma/[email protected]
Please let me know if you need help with knowing how to test, you should be able to do something like this: