-
Notifications
You must be signed in to change notification settings - Fork 4
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
sdk update #497
sdk update #497
Conversation
Deployed to https://pr-497-dex-ui.stg.aepps.com |
c3a2d2c
to
5da6e07
Compare
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 is still needed here?
src/components/AboutModal.vue
Outdated
import contractInfo from 'dex-contracts-v2/package.json'; | ||
// TODO: i don't think this is ok |
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.
why shouldn't it be?
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 because package-lock is 1.52 MB. I'm not sure about the best practice here.
Should I add "package.json" to "exports" field of aepp-sdk? Or to export sdk version as a constant? I think the best would be to have a solution that works the same for any npm dependency.
As a workaround, you can import the local package.json but there would be a lower-bound version of the installed sdk.
On the other side, you don't need the whole package.json, just a single field of it, maybe you can retrive it in vue.config.js and copy it to a global definition? Then there would be no extra data in the bundle.
Line 11 in 6895935
definitions['process.env.UNFINISHED_FEATURES'] = parseBool(process.env.VUE_APP_UNFINISHED_FEATURES); |
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.
Yes, but using the package.json should be fine
src/components/AboutModal.vue
Outdated
this.nodeVersion = (await this.sdk.getNodesInPool()).find((node) => node.name | ||
=== this.sdk.selectedNodeName)?.version || '-'; |
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.nodeVersion = (await this.sdk.getNodesInPool()).find((node) => node.name | |
=== this.sdk.selectedNodeName)?.version || '-'; | |
try { | |
this.nodeVersion = await this.sdk.api.getNetworkId(); | |
} catch (e) { | |
this.nodeVersion = '-'; | |
} |
Or maybe you don't need try/catch if you have some node by default.
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.
but this is about the node version, not the networkId
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.
my bad, proposing to use getNodeInfo
instead of getting node info of all nodes
this.nodeVersion = (await this.sdk.getNodesInPool()).find((node) => node.name | |
=== this.sdk.selectedNodeName)?.version || '-'; | |
try { | |
this.nodeVersion = (await this.sdk.api.getNodeInfo()).version; | |
} catch (e) { | |
this.nodeVersion = '-'; | |
} |
src/components/AboutModal.vue
Outdated
import contractInfo from 'dex-contracts-v2/package.json'; | ||
// TODO: i don't think this is ok |
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.
Yes, but using the package.json should be fine
42aab5e
to
32f1810
Compare
fixes #357
temp todo list: