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

Fix db init #95

Merged
merged 8 commits into from
Aug 29, 2018
Merged

Fix db init #95

merged 8 commits into from
Aug 29, 2018

Conversation

orlandodemauro
Copy link
Contributor

fixes #94

@Ivshti the version 3.25.2 is broken due to a change in the levelup constructor API.

for more info https://github.com/Level/levelup/blob/master/UPGRADING.md

@Ivshti
Copy link
Owner

Ivshti commented Jul 24, 2018

alright,

Is this backwards-compatible? Or should it be a different major version

@orlandodemauro
Copy link
Contributor Author

@Ivshti from level up readme

You should now do (for identical functionality):

It should be backwards-compatible. But, now that you are asking, maybe a major version would be better to avoid issues

@maxwondercorn
Copy link

Definitely a major version bump

@chrisgbk
Copy link

chrisgbk commented Jul 25, 2018

This will break level-js compatibility, because the db parameter of the options is no longer used to specify the underlying database used by levelup - this is the point of the API change in levelup. Also recommend the options get passed to encode so encoding options can be specified, in the same manner as the level package via level-packager does. Something along the lines of:

  var options = this.options.store || { };
  var db = options.db || leveldown;
  this.store = stores[path.resolve(filename)] = (this.store && this.store.isOpen()) ? this.store : levelup(encode(db(filename), options), options);

@orlandodemauro
Copy link
Contributor Author

@chrisgbk totally agree and thanks for the hint.

@Ivshti I applied the changes, could you please merge and publish a new version?

thanks all guys

@c10h22
Copy link
Contributor

c10h22 commented Jul 26, 2018

Hi,

I just noticed that you have add leveldown dependency in your PR which give me the following error on install Module not found: Error: Can't resolve 'leveldown' in '/xxxxxxxxx/node_modules/linvodb3/lib'

Does upgrading oblige user to move on to leveldown in project where we are using level-js instead ?

@orlandodemauro
Copy link
Contributor Author

orlandodemauro commented Jul 26, 2018

@c10h22 no it doesn't. Just try to do a clean install of the module. @Ivshti could you please merge and bump the version please? The last version of the package is broken

@chrisgbk
Copy link

@orlandodemauro I think the changes to model.js forces leveldown to exist, since it now gets required - this is a problem in environments where leveldown is unavailable (ie: pure JS with no binaries).

Quick fix should be to wrap a try-catch block around just the leveldown require, then change the initStore function to throw an error if db is null (leveldown is unavailable, and no alternative is provided).

@c10h22
Copy link
Contributor

c10h22 commented Jul 27, 2018

@orlandodemauro and maybe move leveldown to devDependecies instead of dependencies for those who are actually using level-js

c10h22 added a commit to c10h22/linvodb3 that referenced this pull request Jul 27, 2018
This is actually retro-compatible modification that fixes Ivshti#94 and is related to Ivshti#95
@Ivshti Ivshti merged commit 6952de3 into Ivshti:master Aug 29, 2018
@wittyPuneet
Copy link

Has this commit been pushed to NPM? I still get an old version of Model.js when doing npm install.

@Akumzy
Copy link

Akumzy commented Sep 7, 2018

When will it be publish ti NPM? I'm still facing same issue #101
please...

@Ivshti
Copy link
Owner

Ivshti commented Sep 7, 2018

done, published v3.26.0: https://www.npmjs.com/package/linvodb3

@Akumzy
Copy link

Akumzy commented Sep 7, 2018

thank you so much..

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.

InitializationError:
7 participants