-
Notifications
You must be signed in to change notification settings - Fork 38
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
[TS] Refactor TS module #759
Conversation
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.
This is good, but the changes seem like the content of two PRs.
Do you mean the edit: @bh2smith I reduced it as much as possible while keeping it buildable with the new structure. Let me know if its easier to review :). |
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.
Seems fine. Let's coordinate on how to merge this with the other outstanding ts paths
package.json
Outdated
@@ -2,10 +2,10 @@ | |||
"name": "@gnosis.pm/dex-contracts", | |||
"version": "0.2.15", | |||
"description": "Contracts for dFusion multi-token batch auction exchange", | |||
"main": "src/index.js", | |||
"main": "build/commonjs/index.ts", |
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.
Isn't the path just common and doesn't the file still end in js?
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 path was wrong, I noticed it when I used yarn link
with dex-price-estimator
.
src/encoding.d.ts
Outdated
@@ -0,0 +1,20 @@ | |||
import BN from "bn.js"; |
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.
Will this conflict with #753 ?
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.
You should be able to just remove the encoding.d.ts
file once encoding.js
becomes encoding.ts
.
4751213
to
3eb5972
Compare
Looks like |
a53b487
to
e4c20ac
Compare
96b9c11
to
c9066d0
Compare
@@ -8,7 +8,7 @@ | |||
// "allowJs": true, /* Allow javascript files to be compiled. */ | |||
// "checkJs": true, /* Report errors in .js files. */ | |||
// "jsx": "preserve", /* Specify JSX code generation: 'preserve', 'react-native', or 'react'. */ | |||
// "declaration": true, /* Generates corresponding '.d.ts' file. */ | |||
"declaration": true, /* Generates corresponding '.d.ts' file. */ |
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.
What are these .d.ts
files anyway?
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.
Type definitions. They allow other TypeScript projects to use type information from the original TS code.
This refactors the main module so it is now a TS module. This means that TS projects (like dex-price-estimator) should integrate better with the contracts as it is now a TS module.
Test Plan
Use
yarn link
with a TS project and ensure that this new module system works as intended. I will add detailed instructions soon.