-
Notifications
You must be signed in to change notification settings - Fork 82
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
Update Quiet client to detect a v2 invite link format #2330
Conversation
…n, serverAddress, inviterAddress) invitation link format
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.
Thanks so much, overall looks great
import { QUIET_JOIN_PAGE } from './static' | ||
import { createLibp2pAddress, isPSKcodeValid } from './libp2p' | ||
// import { CID } from 'multiformats/cid' // Fixme: dependency issue |
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 the issue?
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.
If you'd uncommented this line, added 'multiformats' to package.json and install deps you'd get
ERROR in ../common/lib/invitationCode.js 10:14-41
Module not found: Error: Package path ./cid is not exported from package /(...)/monorepo/packages/common/node_modules/multiformats (see exports field in /(...)/monorepo/packages/common/node_modules/multiformats/package.json)
@ ../common/lib/index.js 17:13-40
@ ./src/nest/app.module.ts 29:0-44 91:32-43
@ ./src/backendManager.ts 6:0-46 36:59-79 76:59-79
while building backend package.
Now - multiformats package supports only ESM modules. All of our modules are declared as ES but in common package we were missing explicit declaration. Thought that maybe it inherited commonjs from root tsconfig.
Added explicit declaration to tsconfig. Build error disappeared but now I get:
FAIL src/invitationCode.test.ts
● Test suite failed to run
Cannot find module 'multiformats/cid' from 'invitationCode.ts'
Require stack:
invitationCode.ts
invitationCode.test.ts
2 | import { QUIET_JOIN_PAGE } from './static'
3 | import { createLibp2pAddress, isPSKcodeValid } from './libp2p'
> 4 | import { CID } from 'multiformats/cid'
while running tests in common package.
Before you ask - yes, file does multiformats/cid exist in common package's node_modules.
I'm probably missing some configuration or part of my config is invalid.
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.
When you add that package to package.json are you on the same version that we're on in backend? And just to be sure its a dependency and not a devDependency, right?
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.
It sounds like I had a similar issue with the '@ipld/dag-cbor' package.
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 tried it locally and it worked. Steps I took:
- From root directory run
npm run lerna -- add --scope=@quiet/common multiformats
- Uncomment that line
- From root directory run
npm run lerna bootstrap
- From common directory run
npm run build
to make sure it compiles - From common directory run
npm run test
All tests passed for me. I tested with the latest version of multiformats which is major version 13 (the version that's automatically added to backend seems to be version 12).
NOTE: Manually compiling common shouldn't be necessary since it's done in bootstrap but I wanted to be as thorough as possible.
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.
It doesn't work when you also uncomment usage of CID
@@ -224,6 +224,8 @@ export const PerformCommunityActionComponent: React.FC<PerformCommunityActionPro | |||
// TODO: What does this mean and do we need this here? It might make | |||
// sense to decouple the PSK from this component, since this is the | |||
// only place PSK is used. | |||
|
|||
// TODO: adjust to v2 link, maybe pass already composed link instead of data |
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 needs to be done here in regards to this?
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'll just remove code responsible for locking form. It hasn't been used since orbitdbidentity was added. One added ownerOrbitDbIdentity to PerformCommunityActionComponent props but did not pass down value from JoinCommunity.
|
||
console.log('join network saga payload', payload) | ||
yield* put(communitiesActions.createNetwork(payload)) | ||
} |
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'd probably put this into createNetwork, since we aren't really joining a network here
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 may be right. I added new saga because new flow of joining community requires some additional steps (that would be done as part of the next task). So right now it seems like I'm just creating payload object for createNetwork but I separated it from createNetwork with next steps in mind.
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 most of the joining is going to occur in launchCommunity
, while createNetwork
is still just creating a Tor site.
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 we can deal with renaming things later when we see how it all fits together too
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.
Resigning from joinNetwork saga is also fine by me if it would fit better with your refactoring.
…a; Move parsing code from main.ts to saga
Pull Request Checklist
(Optional) Mobile checklist
Please ensure you completed the following checks if you did any changes to the mobile package: