Some issues and threads #358
Conversation
| - dependency-cache-{{ checksum "package.json" }} | ||
| # fallback to using the latest cache if no exact match is found | ||
| - dependency-cache- | ||
| #- restore_cache: |
There was a problem hiding this comment.
Hmm, are you sure we want to disable the cache?
There was a problem hiding this comment.
Oh, just saw this comment:
Disabling CI
node_modulescaching to solve failure aftertruffle-hdwallet-providerupgrade. (Refs: https://circleci.com/gh/tasitlabs/TasitSDK/593)
There was a problem hiding this comment.
Should we consider downgrading that package instead? Will we be able to re-enable caching at any point?
There was a problem hiding this comment.
We can downgrade truffle-hdwallet-provider package or try to re-enable in the future.
There was a problem hiding this comment.
What in the error message indicated to you that it was realted to truffle-hdwallet-provider?
> npx lerna bootstrap --hoist
lerna notice cli v3.13.4
lerna info ci enabled
lerna info Bootstrapping 6 packages
lerna info Installing external dependencies
lerna info hoist Installing hoisted dependencies into root
lerna info hoist Pruning hoisted dependencies
lerna info hoist Finished pruning hoisted dependencies
lerna ERR! npm install --no-save exited 1 in 'root'
lerna ERR! npm install --no-save stderr:
npm ERR! path /home/circleci/tasit-sdk/node_modules/web3-providers-ws/node_modules/websocket
npm ERR! code EISGIT
npm ERR! git /home/circleci/tasit-sdk/node_modules/web3-providers-ws/node_modules/websocket: Appears to be a git repo or submodule.
npm ERR! git /home/circleci/tasit-sdk/node_modules/web3-providers-ws/node_modules/websocket
npm ERR! git Refusing to remove it. Update manually,
npm ERR! git or move it out of the way first.
There was a problem hiding this comment.
The results you shared are convincing - I think lerna's docs are wrong, then. Because it says that it hoists "shared" dependencies.
There was a problem hiding this comment.
I made this issue on the lerna repo lerna/lerna#2062
There was a problem hiding this comment.
packages/tasit-action/node_modules:
tasit-account tasit-contractsThis one is surprising then, right?
I didn't get it. tasit-action is using tasit-contracts to resolve contracts ABI/address and tasit-account as devDependency to access createFromPrivateKey helper.
There was a problem hiding this comment.
The issue to enable cache in future: #359
There was a problem hiding this comment.
packages/tasit-action/node_modules:
tasit-account tasit-contractsThis one is surprising then, right?
I didn't get it.
tasit-actionis usingtasit-contractsto resolve contracts ABI/address andtasit-accountas devDependency to accesscreateFromPrivateKeyhelper.
Yep. So why aren't they hoisted like all of our other deps?
| @@ -108,6 +108,10 @@ export class Action extends Subscription { | |||
| const baseEthersListener = async blockNumber => { | |||
| try { | |||
| const tx = await this.#tx; | |||
| if (!tx) { | |||
| console.log(`The action wasn't sent yet.`); | |||
There was a problem hiding this comment.
How does this log statement in a dependency (our own) get handled with our removing log approach in the Tasit apps?
There was a problem hiding this comment.
Oh, I should have used the warn instead.
The console removal plugin used on tasit removes all console.* from code.
Not sure about the best way to deal with warn, not sure if make sense use error event for that. What do you think?
There was a problem hiding this comment.
Oh yes, that's a good point that this should be warn.
If this is something that would prevent the tx from going through, then passing this back to the caller as an error so they can retry the tx and update the UI accordingly probably makes sense.
There was a problem hiding this comment.
Oh, just thought again about it: It's happening when an action starts to be listening before send it (the tx field is assigned on the send function). I think that worths a test case, and maybe is a situation to be ignored?
There was a problem hiding this comment.
Yeah, that's worth a test case
I'm not sure if it's a situation to be ignored, though? Why would it be okay to ignore it?
There was a problem hiding this comment.
I've wrote this test case:
it("should be able to listen to an event before sending", async () => {
const confirmationListener = sinon.fake(async message => {
action.off("confirmation");
});
const errorListener = sinon.fake();
action = sampleContract.setValue(rand);
action.on("error", errorListener);
action.on("confirmation", confirmationListener);
await mineBlocks(provider, 2);
await action.send();
await action.waitForOneConfirmation();
await mineBlocks(provider, 2);
expect(confirmationListener.callCount).to.equal(1);
expect(errorListener.called).to.be.false;
});That way, warn is being printed for each new block mined, I'm not sure if it's desirable.
Since:
- It's expected behavior;
- I've improved the
signAndSendfunction (that assigns thethis.#txfield) to guarantee that it'll emit an error event if some error occurs;
Maybe we could remove this warn.
There was a problem hiding this comment.
Should this gate merging?
I don't think so.
There was a problem hiding this comment.
Ahh ok - I forgot that the action not being sent yet was something we would want to happen sometimes.
Based on that, what I said earlier:
If this is something that would prevent the tx from going through, then passing this back to the caller as an error so they can retry the tx and update the UI accordingly probably makes sense.
...didn't make sense. So I'm glad you wrote this test as a way of clarifying how we would run into this scenario. Thanks!
There was a problem hiding this comment.
That way, warn is being printed for each new block mined, I'm not sure if it's desirable.
Yep, this is an argument for moving it back to INFO level
There was a problem hiding this comment.
- I've improved the
signAndSendfunction (that assigns thethis.#txfield) to guarantee that it'll emit an error event if some error occurs;
Oh okay, great!
Maybe we could remove this warn.
Oh remove it, not just change it to an INFO log? Maybe if we INFO log when sending and INFO log when listening we could remove it here.
|
I think it's ready to merge. |
| @@ -34,7 +34,7 @@ jobs: | |||
| # Download and cache dependencies | |||
| #- restore_cache: | |||
| # keys: | |||
| # - dependency-cache-{{ checksum "package.json" }} | |||
| # - dependency-cache-{{ checksum "package-lock.json" }} | |||
| # fallback to using the latest cache if no exact match is found | |||
There was a problem hiding this comment.
Seems like we definitely won't want to enable this
Issue link
#351
#350
#349
#298
#291
#329
Auto-close the issue?
Closes #351
Closes #350
Closes #349
Closes #298
Closes #291
Closes #354 (comment)
Types of changes
Technical debt (a code change that doesn't fix a bug or add a feature but makes something clearer for devs)
Notes
Disabling CI
node_modulescaching to solve failure aftertruffle-hdwallet-providerupgrade. (Refs: https://circleci.com/gh/tasitlabs/TasitSDK/593)