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

Changes to make easier to use PouchDB on Node on Windows #3113

Closed
wants to merge 5 commits into from
Closed

Changes to make easier to use PouchDB on Node on Windows #3113

wants to merge 5 commits into from

Conversation

jacargentina
Copy link
Contributor

Implements needed changes for #3110

Frank Rousseau and others added 5 commits September 10, 2014 01:45
I add this test because currently when a filtered replication is running, if an
empty batch is returned (no remote document match the filter for this batch),
the replication stops. The expected behavior is that the replication keeps
running until it hits the last document sequence.
@nolanlawson
Copy link
Member

This seems to be failing due to jshint, but I really like the approach. I'm +1 after jshint fixes and a green run. Also squashing the commits would be super. :)

@calvinmetcalf could you comment on this "optional dependency" stuff? Looks fine to me; users are at least warned about the lack of a leveldown dependency, although maybe we should emit('error') or something?

@NickColley
Copy link
Contributor

This is a great addition thanks @jacargentina, I've had problems building in Windows in the past too, when you're just doing browserify stuff it's a pain.

@daleharvey
Copy link
Member

Huge +1 for this, will wait for calvins response, seems like we dont need the try {} since nothing should throw with the require.resolve but otherwise will be very happy to get this in

@jacargentina
Copy link
Contributor Author

@daleharvey i've looked at

http://stackoverflow.com/a/15303236/2917185

It seems it throws an error, when the module is not found

@marten-de-vries
Copy link
Member

I like the idea, but isn't that require.resolve if statement useless? My understanding: the file adapters/leveldb.js will always be there. Leveldown might not be, but that error will only be thrown by the require call inside the if statement (or actually by the code that is required). Correct?

@jacargentina
Copy link
Contributor Author

Pouch will not work at all if the IF is removed. Try it!

@marten-de-vries
Copy link
Member

@jacargentina It works here. Maybe we're doing something different?

This might be even more flexible if only the require() of leveldown in leveldb.js would be wrapped in a try/catch, then e.g. memdown/sqldown/etc. would still work. I'll see if I can get a branch with that up...

@nolanlawson
Copy link
Member

This might be even more flexible if only the require() of leveldown in leveldb.js would be wrapped in a try/catch, then e.g. memdown/sqldown/etc. would still work. I'll see if I can get a branch with that up...

+1, this is definitely needed. E.g. @yaronyg has been trying to build PouchDB on exotic embedded platforms, and needed a way to just use SQLdown or just use medeadown etc.

@marten-de-vries
Copy link
Member

OK, so the branch that probably was just linked to by github produces the following:

marten@marten-laptop:~/git/pouchdb$ node
> var PouchDB = require('./')
undefined
> var db = new PouchDB('test')
undefined
> db.allDocs().catch(console.log.bind(console))
[object Object]
> [Error: leveldown not available (specify another backend using the db option)]

undefined
> var db2 = new PouchDB('test', {db: require('memdown')})
undefined
> db2.allDocs().then(console.log.bind(console))
[object Object]
> { total_rows: 0, offset: 0, rows: [] }

undefined
> 

Might be better if the var db = new PouchDB('test') line already threw an exception, but this seems in line with how websql handles an error at this stage of the instantiation process. Plus, I wasn't sure how to do that this quickly.

PouchDB.adapter('ldb', ldbAdapter);
PouchDB.adapter('leveldb', ldbAdapter);
}
try {
Copy link
Member

Choose a reason for hiding this comment

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

wrap this in a (function () {}()) to avoid v8 deopt

@jacargentina
Copy link
Contributor Author

Somebody please take the idea and code as discussed/suggested... I dont have that much knowledge, v8 deopt thing.. Etc feel free to close this PR and do your own. Hope this gets merged and next release!

@nolanlawson
Copy link
Member

Calvin's referring to the fact that try/catch gets de-optimized unless you put it inside its own function. We got your back, though, no prob. :)

@marten-de-vries
Copy link
Member

I just opened #3140 which does the try/catch wrapping & also supports other leveldb backends like memdown.

@marten-de-vries
Copy link
Member

#3140 has been merged, so I'm closing this. Thanks @jacargentina for the initial implementation & everyone for the input on this.

@nolanlawson nolanlawson mentioned this pull request Dec 27, 2014
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.

None yet

6 participants