fix: add backwards compatibility and more tests #138
Conversation
|
Me and @dignifiedquire had a quick chat yesterday, but unfortunately, I wasn't able to follow-up at the time as I was entering a flight. I've run js-ipfs tests with latest ipfs-repo and they finished successfully, this was a false positive because as @dignifiedquire points out, there were breaking changes with regards to the options object, but since everyone repo uses ipfs-repo defaults, it felt like an internal breaking change. What we should do in this situation is add backward's compatibility to the old options object, this will enable old custom configs to work if anyone is using any. @dignifiedquire can you refactor your PR to be that? With regards to other possible breaking changes. I'm still unsure what those might be as there are no tests that capture those. Mind creating an issue to enumerate those? We should have tests for them in the first place. |
|
Things to fix:
We need tests for these also. |
daviddias
changed the title
| @@ -40,7 +43,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "aegir": "^11.0.2", | |||
| "chai": "^4.0.1", | |||
There was a problem hiding this comment.
Interesting, why the downgrade?
package.json
Outdated
| "lock-me": "^1.0.2", | ||
| "multiaddr": "^2.3.0", | ||
| "safe-buffer": "^5.1.0" | ||
| "safe-buffer": "^5.0.1" |
There was a problem hiding this comment.
Ah, it was generalized. Mind doing a ncu -a?
src/index.js
Outdated
| @@ -43,21 +44,15 @@ class IpfsRepo { | |||
| constructor (repoPath, options) { | |||
| assert.equal(typeof repoPath, 'string', 'missing repoPath') | |||
|
|
|||
| if (options == null) { | |||
| options = require('./default-options') | |||
There was a problem hiding this comment.
Changing this means that any module bundle will include the default options in the browser always. Not sure this could be an issue or not
There was a problem hiding this comment.
Since webpack and browserify extract dependencies using static analysis, this is always going to get included in the bundle, with or without these lines.
There were some issues introduced with #136. This should be reverted for now, until it is cleared to not cause any issues.