Skip to content
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

Add Orbit & DDB basics #94

Merged
merged 18 commits into from
May 28, 2018
Merged

Add Orbit & DDB basics #94

merged 18 commits into from
May 28, 2018

Conversation

laurentsenta
Copy link

Description

Deps

New dependencies:

  • ipfs : Storage used by orbit-db.
  • orbit-db: Decentralized database

@laurentsenta laurentsenta changed the title Feature/83 orbit in dapp Add Orbit & DDB basics May 9, 2018
@laurentsenta laurentsenta mentioned this pull request May 9, 2018
4 tasks
@laurentsenta laurentsenta force-pushed the feature/83-orbit-in-dapp branch 5 times, most recently from f5cfa63 to 390b4c5 Compare May 15, 2018 08:47
@laurentsenta laurentsenta requested review from coyotespike and rdig May 15, 2018 09:03
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still very rough. It looks to me like it's still a work-in-progress.

Please see my comments and align this PR with our rules in the codebase.

Please use the included prettier and eslint rules as those will help you avoid most things I've outlined.

👍

@@ -0,0 +1,134 @@
import rimraf from 'rimraf';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put this file under ./integration-testing/utils to not mix it up with the tests

@@ -3,6 +3,9 @@
"rootDir": "integration-testing",
"globalSetup": "./utils/integration-testing-setup.js",
"globalTeardown": "./utils/integration-testing-teardown.js",
"setupFiles": [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going do generate a merge conflict, as I've also added a bunch of files in #93


subscribe(f) {
this._store.events.on('replicated', () => {
// console.log(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this commented out code needed, or just a leftover ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to keep these around to quickly uncomment and track my errors, I'll remove it.

Do we have a preferred logging library that'd let me log debug / info / warn / alert with namespaced loggers that you can enable and disable with a piece of config?

}

async getUserProfile(key: PublicKey): Promise<UserProfile> {
// console.log('Build User Profile Store with key=', key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, are these needed, or can they be removed ?


let update = {};
p2.subscribe(x => {
console.log('Subscription triggered with x=', x);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of console.log statements all over the tests. Do we need this ?

As they seem to me to be more for a debugging purpose.

import { retryUntilValue } from '../utils/tools';

let factory = null;
let pinner = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is not used in this file.

await factory.clear();
}, Factory.TIMEOUT);


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the extra line here.


test('Create my user profile and set its name', async () => {
const p1 = await data2.getMyUserProfile();
let name = factory.name('Kanye West');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name is never reassigned, please use const

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have a linter precommit hook? That'd be a good thing to set up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Author

@laurentsenta laurentsenta May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I passed yarn lint before pushing.
We need to add integration-testing/.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good point 👍

Although, curiously, my editor's linter (which takes it's rules from the local project's .eslintrc) works...

expect(p1.getName()).toBe(name);
});

test('Create my user profile and set its name sync with another', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line exceeds the max length of 80.

Please use the included prettier and eslint rules.


expect(p1.isEmpty()).toBeFalsy();
expect(await retryUntilValue(() => p2.isEmpty(), { value: false })).toBeFalsy();
expect(await retryUntilValue(() => p2.getName(), { value: name })).toBe(name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines exceed the max length of 80.

Please use the included prettier and eslint rules.

@laurentsenta laurentsenta force-pushed the feature/83-orbit-in-dapp branch from 42d84ff to 1d3ef14 Compare May 21, 2018 10:06
@laurentsenta
Copy link
Author

laurentsenta commented May 21, 2018

@rdig I yarn lint before pushing (with integration-testing/ folders)
but that doesn't seem enough.

And yarn prettier --write ./integration-testing/**/*.js

doesn't follow our coding style (it puts " quotes everywhere for example).

what's your prettier setup?

@rdig
Copy link
Member

rdig commented May 21, 2018

what's your prettier setup?

prettier is built in our lint setup, so you should just have to run yarn lint and that will do the trick. Although I think you're right, as that one doesn't take into account the integration-tests/ folder.

I'll add it in and fix it in #112

@laurentsenta laurentsenta force-pushed the feature/83-orbit-in-dapp branch from 1d3ef14 to 4b82688 Compare May 21, 2018 12:37
@rdig
Copy link
Member

rdig commented May 21, 2018

Added in #112

There's a manual command to lint integration tests files: yarn test:integration.

But everything is wired up to check these at commit time.

@laurentsenta laurentsenta force-pushed the feature/83-orbit-in-dapp branch from 4b82688 to 7f95222 Compare May 22, 2018 09:36
@laurentsenta
Copy link
Author

@rdig thanks for the thorough review, my editors are now configured with our complete coding style and I fixed all your comments ;)

Could you take another look?

Also, do you Squash & Rebase or just Merge? I kept small & focused commits to make the review easier for now.

@rdig
Copy link
Member

rdig commented May 22, 2018

Could you take another look?

I'll try to make some time today

Also, do you Squash & Rebase or just Merge? I kept small & focused commits to make the review easier for now.

As long as the commits are made with common sense we don't squash, just merge. I we do ever decide to squash, it's going to be brought up in the review.

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is good to go now.

Tests output it much cleaner:

screenshot from 2018-05-22 16-50-57

There's still some eslint errors left, but these have to do with rule config, rather than code: (this is fixed in #112 so just ignore it for now)

screenshot from 2018-05-22 16-50-31

PS: Please rebase on the latest master and re-run tests with the updated versions of the submodules to make sure they still pass

Nice work Laurent 💯

@rdig rdig removed the under-review label May 22, 2018
@laurentsenta laurentsenta force-pushed the feature/83-orbit-in-dapp branch from 7f95222 to de2bfdd Compare May 23, 2018 18:03
@laurentsenta
Copy link
Author

@rdig Arr, the PR is ready to merge, but something broke with the Colony Client apparently:
https://circleci.com/gh/JoinColony/colonyDapp/506?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

@rdig
Copy link
Member

rdig commented May 23, 2018

There's a lot of things that seem to be failing:

  • colony client (i assume this is due to jest)
  • linting on the main circle workflow, most likely we forgot something that eslint picked up

I'll take a look tommorow at it and possibly fix it.

@rdig rdig force-pushed the feature/83-orbit-in-dapp branch from b922674 to 5b7f15d Compare May 24, 2018 09:29
@rdig rdig force-pushed the feature/83-orbit-in-dapp branch 2 times, most recently from bf78f62 to ed14f4d Compare May 24, 2018 09:44
@rdig
Copy link
Member

rdig commented May 24, 2018

I've fixed CI tests, but I still get this failure when running integration tests:

screenshot from 2018-05-24 12-09-16

Can you please investigate ?

PS: I've added a new commit, so make sure to rebase if you're working locally 👍

@laurentsenta
Copy link
Author

Yea, there's a bug in the libp2p suite,

The tests show this error but succeed anyway?
Could we merge this and progress on the DAPP while ipfs-pubsub-room fix the issue?

@rdig
Copy link
Member

rdig commented May 24, 2018

The tests show this error but succeed anyway?

Yes because the console method call fails, not an expect() assertion, so from jest's point of view everything works fine.

Could we merge this and progress on the DAPP while ipfs-pubsub-room fix the issue?

Yes, but add a @TODO comment to the code block that fails, so we can keep track of it.

@laurentsenta laurentsenta force-pushed the feature/83-orbit-in-dapp branch from ed14f4d to 58094b4 Compare May 28, 2018 09:44
@laurentsenta laurentsenta force-pushed the feature/83-orbit-in-dapp branch from 5c6ab92 to 9513849 Compare May 28, 2018 12:57
@rdig rdig merged commit b801772 into master May 28, 2018
@rdig rdig deleted the feature/83-orbit-in-dapp branch May 28, 2018 13:07
@laurentsenta laurentsenta added this to the Sprint 2 milestone May 28, 2018
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.

4 participants