Skip to content

Conversation

@Lezek123
Copy link
Contributor

@Lezek123 Lezek123 commented Jul 28, 2020

Related issue:
#1002

Currently membershipById map seems to just return an empty Membership instance in case member doesn't exist.
Since this instance contains Bool field (Membership.suspended), we can't check if it's empty via Membership.isEmpty, because it will always return false. The current workaround to check for "emptiness" of the Membership struct is to use ie. Membership.handle.isEmpty. Perhaps there will be better ways to handle such cases in the latest version of @polkadot/api.

@Lezek123 Lezek123 requested a review from mnaamani July 28, 2020 18:08
@Lezek123
Copy link
Contributor Author

Bumped the version of @joystream/types and adjusted the dependencies in order to fix network-tests CI checks.

@mnaamani
Copy link
Member

mnaamani commented Aug 6, 2020

I guess the check Membership.handle.isEmpty will do for now and waiting to see what you find in latest version of polkadotjs api, but if the result is similar, ie. is there is no helper to check if a key exists in a runtime map, we should consider the general solution for all maps to get the raw value from storage, because it seems sub-optimal and even fragile each time to try to find which field to do an isEmpty check on.

const key = api.query.members.membershipById.key(memberId)
const maybeValue = (await api.rpc.state.getStorage(key)) // Option<ValueType>
const value = maybeValue.isSome ? new ValueType(maybeValue.unwrap()) : undefined

Tested working as expected

@mnaamani
Copy link
Member

mnaamani commented Aug 6, 2020

The npm pack test for the cli still seems to be failing:

 npm ERR! code ETARGET
npm ERR! notarget No matching version found for @joystream/types@^0.13.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.
npm ERR! notarget 
npm ERR! notarget It was specified as a dependency of 'package'
npm ERR! notarget 

@Lezek123
Copy link
Contributor Author

Lezek123 commented Aug 6, 2020

The npm pack test for the cli still seems to be failing

I was trying to make all the CI chceks work, but this one now fails because the types are not yet published (I bumped the version for Alexandria).
This was a useful check before publishing the CLI, but now I consider removing it or making it "optional" if that's possible.
Or perhaps we should just perform it on master and make sure the types are published before there's a PR to master?

@mnaamani
Copy link
Member

mnaamani commented Aug 6, 2020

Or perhaps we should just perform it on master and make sure the types are published before there's a PR to master?

I think that would be good, but it would be good to find a way to test it before publishing. Is this something lerna can do, or perhaps a similar command in yarn, like yarn pack which would be able to use types from the workspace?

@mnaamani
Copy link
Member

mnaamani commented Aug 6, 2020

I tried locally yarn pack and yarn link and it seems to work.
So maybe for master branch we can do the check with npm, and for PRs and other branches we use yarn?

@Lezek123
Copy link
Contributor Author

Lezek123 commented Aug 6, 2020

The CI check fails after yarn link even though it says that the package is correctly linked (cannot find command joystream-cli): https://github.com/Lezek123/substrate-runtime-joystream/runs/952497411?check_suite_focus=true

I had the same issue locally when using the default configuration (without .npmrc)

@mnaamani
Copy link
Member

mnaamani commented Aug 6, 2020

It seems to work on the mac but fails on ubuntu. Maybe we skip the joystream-cli help step for now. The key check is that it builds and all dependencies from npm are found.

@Lezek123
Copy link
Contributor Author

Lezek123 commented Aug 6, 2020

Adjusted the CI checks without joystream-cli help for now

@mnaamani
Copy link
Member

mnaamani commented Aug 6, 2020

Does the failing of network tests checks have anything to do with merging of #1091

Specifically this: 87c14f6

cc: @gleb-urvanov

@Lezek123
Copy link
Contributor Author

Lezek123 commented Aug 6, 2020

I think it has to do with the version of @nicaea/types inside network-testing package.json (that I set to ^0.12.0 in this PR) and also this: #1085

I'll try to resolve.

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 this pull request may close these issues.

2 participants