-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
@hugomrdias please use |
lol i did just didn't save the file :/ fixed with the last commit |
23138ad
to
4f73f22
Compare
@hugomrdias everything is failing 😞 - would you mind taking a look? |
did someone merged or rebased this branch ? |
4f73f22
to
ee1290d
Compare
Updated aegir to make tests run and rebased with master. Please don't rebase branches you don't own. |
@hugomrdias I don't think anyone rebased, otherwise you would see double authors. |
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.
Please rebase master onto this branch and get CI to be green
ping @hugomrdias can we get this PR in order? People are reporting duplicates of issues that this PR will fix. |
ee1290d
to
6a405c3
Compare
fix: deps semver fix: remove safe-buffer fix: use defaults-deep instead of deep-extend fix: rebase, sort webrtc and add more paths to dep-check fix: change to peer deps
6a405c3
to
cd12d56
Compare
tests passing ship it |
"peerDependencies": { | ||
"electron-webrtc": "~0.3.0", | ||
"wrtc": "~0.2.1" | ||
}, |
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.
We get the following warning now when installing dependencies for js-ipfs:
npm WARN [email protected] requires a peer of electron-webrtc@~0.3.0 but none is installed. You must install peer dependencies yourself.
npm WARN [email protected] requires a peer of wrtc@~0.2.1 but none is installed. You must install peer dependencies yourself.
This makes people think that those dependencies are required when they are not. @achingbrain @hugomrdias do you think these should be moved to optionalDependencies
instead?
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.
With optional they get installed and Jenkins goes crazy 😑😑 . It's missing some lib to compile
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 shouldn't break...reading the docs, that seems to be the whole point of optionalDependencies
...
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.
Ok, remove them for now as they were before...?
This PR also add a npm script to check missing dependencies this should in the future go into aegir.