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 hyperdb support and run tests for hypercore, hyperdb, and hyperdrive #1

Merged
merged 12 commits into from Feb 10, 2019

Conversation

Projects
None yet
2 participants
@aral
Copy link
Contributor

aral commented Feb 2, 2019

This pull request adds hyperdb support. It also abstracts out the tests and runs them once for each database type so we have full coverage of supported database types.

aral added some commits Feb 2, 2019

@@ -24,6 +24,8 @@
},
"homepage": "https://github.com/beakerbrowser/dat-ephemeral-ext-msg#readme",
"dependencies": {
"hypercore": "^6.24.0",
"hyperdb": "github:aral/hyperdb#local-options",

This comment has been minimized.

@pfrazee

pfrazee Feb 2, 2019

Member

Looks good except this -- does it need to be on your branch? Also can it be a dev dep?

This comment has been minimized.

@aral

aral Feb 2, 2019

Author Contributor

Oh gosh, sorry, forgot about that. Will fix that now. Give me a few minutes to go upstairs :)

This comment has been minimized.

@aral

aral Feb 2, 2019

Author Contributor

Right, so that broke the tests! 🙄

I think it might be an incompatibility between the version of hypercore/hypercore protocol used by hyperdb and hyperdrive when included in the same app. I had the tests break before and nuking the node_modules directory and npm i got them working again then. But it’s clearly fragile. I’m going to look into that now. Will update once I have something that works reliably across various versions.

Where would be a good place to raise the issue of cross-package compatibility? It would make sense that hyperdb and hyperdrive both used the same version of hypercore and the hypercore protocol.

Update: Please see last comment for actual reason and latest commit for the fix.

module.exports = function () {

var tape = require('tape')
var database = require('hyperdrive')

This comment has been minimized.

@aral

aral Feb 2, 2019

Author Contributor

Also, I’d forgotten this here and wasn’t actually passing the database in so I was testing hyperdrive three times (fun). Fixed that but tests still failing. Very odd. Something is fragile but I don’t know what. Going to keep looking.

@aral aral changed the title Add hyperdb support and run tests for hypercore, hyperdb, and hyperdrive WIP: Add hyperdb support and run tests for hypercore, hyperdb, and hyperdrive Feb 2, 2019

@aral

This comment has been minimized.

Copy link
Contributor Author

aral commented Feb 3, 2019

So it was a timing issue. Looks like the clone’s peers were not being populated (sometimes?… as I swear I saw it pass earlier) by the time the streams handshake event had fired. Running the tests on the next tick solved the issue. All tests passing again.

Please feel free to merge at your leisure unless you find any other issues. I’m happy with it :)

@aral aral changed the title WIP: Add hyperdb support and run tests for hypercore, hyperdb, and hyperdrive Add hyperdb support and run tests for hypercore, hyperdb, and hyperdrive Feb 3, 2019

aral added some commits Feb 3, 2019

@aral

This comment has been minimized.

Copy link
Contributor Author

aral commented Feb 10, 2019

Just updated to use the release version of hyperdb (sorry, that slipped my mind again earlier).

I’m happy with this now if you are, @pfrazee :)

@pfrazee

This comment has been minimized.

Copy link
Member

pfrazee commented Feb 10, 2019

@aral Sounds good, I'll test it in Beaker. One other nit - you're using the variable name database as a catchall for different dat structures. That does makes sense, but I'd prefer to use dat instead. Would you mind changing it?

@pfrazee

This comment has been minimized.

Copy link
Member

pfrazee commented Feb 10, 2019

👍 Beaker tests pass

@aral

This comment has been minimized.

Copy link
Contributor Author

aral commented Feb 10, 2019

I'd prefer to use dat instead. Would you mind changing it?

Sure, done :)

@pfrazee pfrazee merged commit f27eb09 into beakerbrowser:master Feb 10, 2019

@pfrazee

This comment has been minimized.

Copy link
Member

pfrazee commented Feb 10, 2019

Thanks! Merged and published 1.0.2

@aral

This comment has been minimized.

Copy link
Contributor Author

aral commented Feb 10, 2019

Thanks, Paul. Hope you had a lovely weekend + here’s to hopefully seeing you in Berlin in May :)

@aral aral deleted the aral:hyperdb branch Feb 10, 2019

@pfrazee

This comment has been minimized.

Copy link
Member

pfrazee commented Feb 10, 2019

Same! Looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment