-
Notifications
You must be signed in to change notification settings - Fork 50
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: Upgrade dependencies, use strict and fix /blocks pathing #29
Conversation
what was the pathing issue? |
const PREFIX_LENGTH = 8 | ||
|
||
exports = module.exports | ||
|
||
exports.setUp = (basePath, blobStore, locks) => { | ||
var store = blobStore(basePath + '/blocks') | ||
const store = blobStore(basePath + '/blocks') |
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 was here, in the version that was installed from npm, looks like I didn't fix it but the fix is not yet released to npm
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 said before /blocks/
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'm confused, could you explain a bit more please? btw, travis is failing
@dignifiedquire I'm still unaware of what was the bug this PR is fixing and what is the actual solution. Maybe @nginnever knows the answer? (travis tests are still failing) |
Sorry will explain tomorrow |
@diasdavid the most important fix is adding The other issue was that in an older revision the repo path for the blocks was |
The tests also run in PhantomJS now to ensure things work in older browsers.
Thank you @dignifiedquire . Also good discussion on the call today @dignifiedquire @nginnever :) |
fix: Upgrade dependencies, use strict and fix /blocks pathing
For @nginnever :)